-
Notifications
You must be signed in to change notification settings - Fork 171
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
Comments
4 tasks
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.
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
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:
The problem is that an
Arc<DocInner>
is use to refcount the handle, but instead ofnode.docs.open()
returning a clone of an existingArc<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:
iroh/iroh/src/client/docs.rs
Lines 155 to 162 in d37a4a4
iroh/iroh/src/client/docs.rs
Lines 138 to 149 in d37a4a4
The text was updated successfully, but these errors were encountered: