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

Add Send marker trait to into_reader. #156

Merged
merged 3 commits into from
Sep 27, 2020
Merged

Conversation

jsha
Copy link
Collaborator

@jsha jsha commented Sep 25, 2020

This allows the resulting Read to be shared among threads.

This allows the resulting Read to be shared among threads.
Copy link
Owner

@algesten algesten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes a lot of sense.

I'm unsure about Sync here. That means we guarantee the reader isn't only possible to pass around to other threads, but it's also consistently observable simultaneously in all threads.

In practice our impl maybe always wrap the actual thing in a mutex, so we do guarantee that. I'm thinking maybe we some future impl of our reader has some sweet optimization we can't uphold with Sync which means a breaking change. But maybe that's too much thinking?

Since we don't impl Clone and read() call is using &mut self, in practice it maybe doesn't make much difference?

@rawler maybe has some input? :)

@jsha
Copy link
Collaborator Author

jsha commented Sep 27, 2020

Pushed a change to narrow this to just Send, which is what I need to unblock some work on git2-rs.

In practice it looks like our Read gets Sync because TcpStream, StreamOwned, and Cursor all get it. My thinking was that with dyn, once we've "cast away" the marker traits, it's impossible for user code ever to get them back (as opposed to generics, where the marker traits automatically propagate through). In that context it felt a shame to throw away properties that might be valuable to the user. But I do see your point: the impl Read return hides some of our implementation details (like ChunkDecoder, DeadlineStream, PoolReturnRead) from the user, which gives us flexibility; and one of the things we might want to keep hiding is the Sync'ness so we have flexibility there.

@algesten algesten merged commit 5801f87 into algesten:master Sep 27, 2020
@algesten
Copy link
Owner

Perfect. Let's merge!

@jsha jsha deleted the send-sync branch September 27, 2020 18:25
@jsha jsha changed the title Add Send + Sync marker traits to into_reader. Add Send marker trait to into_reader. Sep 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants