fix: garbage collect Stream when dropped
#167
Conversation
This fixes issue libp2p#166 where a `Stream` would never be removed until its connection is closed.
There was a problem hiding this comment.
Very well done @aschran. Thank you for reporting, debugging and fixing. I appreciate your work here.
I missed this in my review in #156 (comment).
Overall this looks good to me. Missing steps:
- I would like to give @thomaseizinger a chance to review as well.
- @aschran would you mind adding a changelog entry to
CHANGELOG.md?
Again, well done. Thank you.
|
This likely warrants a patch release (i.e. |
|
@aschran @thomaseizinger I merged latest |
Co-authored-by: Max Inden <[email protected]>
Co-authored-by: Max Inden <[email protected]>
Co-authored-by: Max Inden <[email protected]>
|
Committed your suggestions. Thanks for the review and confirmation + test! Will add the CHANGELOG entry shortly when I can get back to my computer. |
There was a problem hiding this comment.
Thank you!
Ironically, the fact that all of Stream was shared bugged me already in the past because it is conceptually hard to understand. The fact that it now actually caused a bug annoys me that I didn't follow my gut feeling there :)
I made one suggestion on how I think we could make this a bit cleaner. Let me know if you are up for it, otherwise happy to merge this as is once CI passes!
| @@ -350,7 +353,7 @@ struct Active<T> { | |||
| socket: Fuse<frame::Io<T>>, | |||
| next_id: u32, | |||
|
|
|||
| streams: IntMap<StreamId, Stream>, | |||
| streams: IntMap<StreamId, Arc<Mutex<stream::Shared>>>, | |||
There was a problem hiding this comment.
I wonder if it would turn out cleaner if Shared would hold Arc<Mutex> internally? That would remove the boilerplate around locking and some noise from the type signatures.
There was a problem hiding this comment.
Would prefer to merge the fix and leave this for future work if that's ok with you, since it looks like a bit more involved of a change
Co-authored-by: Thomas Eizinger <[email protected]>
|
Can you look at the CI failures? Otherwise LGTM! |
I'll admit I'm not sure what's up with those. If I run |
…chran/dropstreams
|
Our CI first merges the target branch, here We recently merged #153. While git does not identify it as a merge conflict, it does conflict with this pull request. I pushed a merge resolving the non-git-merge-conflict conflicts. @thomaseizinger this moves the |
|
Thanks for helping explain & fix Max :) |
There was a problem hiding this comment.
Merging here.
@thomaseizinger let me know in case you would like to see any follow-ups given the latest changes.
@aschran once more, thank you for the work. Let me know in case you would like me to backport this change to v0.11.
mxinden
changed the title
Shared state in Active<T>Stream when dropped
|
@mxinden yes, if you have time to backport that would be great! Appreciate the diligent review from you both. |
None that are critical for a release. |
|
That works, thanks for letting me know! |
This fixes an issue where a
Streamwould never be removed until its connection is closed.Resolves #166.