Skip to content

Commit

Permalink
fix(iroh): do not double-close docs on drop (#2383)
Browse files Browse the repository at this point in the history
## Description

We unconditionally sent a `DocClose` RPC message when dropping a
`iroh::client::docs::Doc` struct. However, we should only ever send a
`DocClose` *once* for each doc instance, because otherwise the reference
counter in the backend will decrease *twice*, and thus closing the
resource in the backend too soon, affecting other open client instances.
This was either an oversight or got lost in refactorings, not sure
anymore. In any case: This PR fixes the behavior, and also adds a test
that fails without this fix.

Fixes #2381 

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->

## Change checklist

- [x] Self-review.
- [ ] ~~Documentation updates if relevant.~~
- [x] Tests if relevant.
- [x] All breaking changes documented.
  • Loading branch information
Frando authored Jun 20, 2024
1 parent f73c506 commit 55a0c0b
Showing 1 changed file with 33 additions and 6 deletions.
39 changes: 33 additions & 6 deletions iroh/src/client/docs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,11 @@ where
fn drop(&mut self) {
let doc_id = self.id;
let rpc = self.rpc.clone();
self.rt.spawn(async move {
rpc.rpc(DocCloseRequest { doc_id }).await.ok();
});
if !self.closed.swap(true, Ordering::Relaxed) {
self.rt.spawn(async move {
rpc.rpc(DocCloseRequest { doc_id }).await.ok();
});
}
}
}

Expand Down Expand Up @@ -176,13 +178,14 @@ where

/// Close the document.
pub async fn close(&self) -> Result<()> {
self.0.closed.store(true, Ordering::Release);
self.rpc(DocCloseRequest { doc_id: self.id() }).await??;
if !self.0.closed.swap(true, Ordering::Relaxed) {
self.rpc(DocCloseRequest { doc_id: self.id() }).await??;
}
Ok(())
}

fn ensure_open(&self) -> Result<()> {
if self.0.closed.load(Ordering::Acquire) {
if self.0.closed.load(Ordering::Relaxed) {
Err(anyhow!("document is closed"))
} else {
Ok(())
Expand Down Expand Up @@ -783,6 +786,30 @@ mod tests {
Ok(())
}

/// Test that closing a doc does not close other instances.
#[tokio::test]
async fn test_doc_close() -> Result<()> {
let _guard = iroh_test::logging::setup();

let node = crate::node::Node::memory().spawn().await?;
let author = node.authors().default().await?;
// open doc two times
let doc1 = node.docs().create().await?;
let doc2 = node.docs().open(doc1.id()).await?.expect("doc to exist");
// close doc1 instance
doc1.close().await?;
// operations on doc1 now fail.
assert!(doc1.set_bytes(author, "foo", "bar").await.is_err());
// dropping doc1 will close the doc if not already closed
// wait a bit because the close-on-drop spawns a task for which we cannot track completion.
drop(doc1);
tokio::time::sleep(tokio::time::Duration::from_millis(100)).await;

// operations on doc2 still succeed
doc2.set_bytes(author, "foo", "bar").await?;
Ok(())
}

#[tokio::test]
async fn test_doc_import_export() -> Result<()> {
let _guard = iroh_test::logging::setup();
Expand Down

0 comments on commit 55a0c0b

Please sign in to comment.