Fix pinned drop violations; make types Unpin where practical#222
Merged
Conversation
* FutureGroup and StreamGroup are now `Unpin` types. At heart, they kinda always were * ChunkedVec is now built around pinning, fulfilling its destiny as a data structure made specifically for pinnable data * Brief changes to tests, to catch the bug that spawned this commit Properly speaking, neither FutureGroup nor StreamGroup need to be pinned. The futures/streams are placed in allocations managed by ChunkedVec, which is responsible for keeping the futures/streams pinned. This has been true. In fact, this means the entire pin_project/`#[pin]` management done by these types was superfluous. FutureGroup and StreamGroup don't store any pinnable data inline. Going forward, these types can be safely exposed as Unpin. Even if their implementation changes again, FutureGroup and StreamGroup are never going to store their streams/futures on stack. Only inline storage makes pinning self-referential data meaningful; heap storage will always be stable. Thus, making them Unpin is totally safe from the perspective of future compatibility. The same goes for derived types. As long as the futures/streams are polled on the heap, there's no way they will need to be pinned in the old-fashioned (inline) way.
Contributor
Author
|
I don't know why, however, MIRI is very slow to run the pin_safety tests. If this is not related to this PR, feel free to disregard. |
If dropping one of the items in ChunkedVec fails from a panic, the other items won't get dropped. So, the memory has to be leaked in order to uphold the Drop guarantee for pinned data.
Owner
|
Thanks for submitting this patch! I thing @matheus-consoli is probably the right person to review this, which is why I've assigned them to it. However if they don't get around to it by the end of next week, please feel free to ping me and I'll do my best to review this. |
matheus-consoli
approved these changes
Jan 22, 2026
matheus-consoli
left a comment
Collaborator
There was a problem hiding this comment.
Hey, thank you so much for fixing this! Everything looks great
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I was writing a library of my own and decided to test this one. Although I had seen that #188 described pinning problems, I only discovered the extent of this myself, when this library began failing my code's anti-move checks.
This PR shores up the pin-on-drop problem. Pinned types have to be dropped in the same place, and ChunkedVec wasn't doing this.
Changes summary
Unpintypes. At heart, they kinda always were (see below)Unpinned collections
Properly speaking, neither FutureGroup nor StreamGroup need to be pinned. The futures/streams are placed in allocations managed by ChunkedVec, which is responsible for keeping the futures/streams pinned. This has been true. In fact, this means the entire pin_project/
#[pin]management done by these types was superfluous. FutureGroup and StreamGroup don't store any pinnable data inline, not even when the implementation used Slab.Going forward, these types can be safely exposed as Unpin. Even if their implementation changes again, FutureGroup and StreamGroup are never going to store their streams/futures on stack. Only inline storage makes pinning self-referential data meaningful; heap storage will always be stable. Thus, making them Unpin is totally safe from the perspective of future compatibility.
Growing streams and polling them too
As a side effect of making FutureGroup and StreamGroup
Unpintypes, they can now have items added to them even after they've been pinned. This was already the case forUnpinitems, but now it will be true for!Unpintypes as well.This was alluded to in #188: by requiring the group to be pinned in order to be polled, the size becomes frozen and new insertions are impossible. I didn't investigate the previous version of futures-concurrency very much, but "freezing" the collection might be the only reason it used pinning.
That said, abandoning that design is surely for the better, and I don't think future implementations of this crate would ever need to bring it back. It was more of a side effect and a quirk of how the crate was implemened.