Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dropping a Doc Handle Closes the Connection on Other Doc Handles That Have Been Independently Opened #2381

Closed
zicklag opened this issue Jun 18, 2024 · 1 comment · Fixed by #2383

Comments

@zicklag
Copy link

zicklag commented Jun 18, 2024

When you open a document it creates a new document handle that, when dropped, closes the document.

This becomes a problem when you open the same document multiple times, because dropping one will try to close the document, while it's still needed by the other handle. Here's an example of failing code:

use iroh::node::Node;

#[tokio::test]
async fn creating_and_dropping_iroh_docs() -> anyhow::Result<()> {
    let node = Node::memory().spawn().await?;
    let id = node.docs.create().await?.id();

    // First we open a document
    let doc1 = node.docs.open(id).await?.unwrap();

    {
        // Then we open the same document again
        let doc2 = node.docs.open(id).await?.unwrap();

        // Let's put some data in it just so we have something to try to read later
        doc2.set_bytes(node.authors.default().await?, "hello", "world")
            .await?;

        // This line seems to be necessary to cause the problem in this minimal example,
        // but I think it is just because of timing.
        doc2.close().await?;

        // I'm pretty sure that when doc2 goes out of scope here, it spawns a task to close
        // the document, like the line above does explicitly.
    }

    // Now when I try to read the data using the doc1 handle, it will error with a 
    // "Replica not open" error.
    let value = doc1
        .get_exact(node.authors.default().await.unwrap(), "hello", true)
        .await
        .unwrap();
    assert_eq!(
        dbg!(value.unwrap().content_bytes(&doc1).await.unwrap()),
        "world"
    );

    Ok(())
}

The problem is that an Arc<DocInner> is use to refcount the handle, but instead of node.docs.open() returning a clone of an existing Arc<DocInner>, it creates a new one. So now we've got two ref counters, and and when the refcount drops to zero on either of them, it closes the document.


Related Code:

fn new(rpc: RpcClient<RpcService, C>, id: NamespaceId) -> Self {
Self(Arc::new(DocInner {
rpc,
id,
closed: AtomicBool::new(false),
rt: tokio::runtime::Handle::current(),
}))
}

impl<C> Drop for DocInner<C>
where
C: ServiceConnection<RpcService>,
{
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();
});
}
}

@Frando
Copy link
Member

Frando commented Jun 18, 2024

You found a bug indeed! #2383 should fix it!

github-merge-queue bot pushed a commit that referenced this issue Jun 20, 2024
## 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.
@github-project-automation github-project-automation bot moved this to ✅ Done in iroh Jun 20, 2024
ppodolsky pushed a commit to izihawa/iroh that referenced this issue Jun 22, 2024
## 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 n0-computer#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants