Skip to content

Add Readable and Writable futures #64

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

Merged
merged 2 commits into from
May 22, 2021
Merged

Add Readable and Writable futures #64

merged 2 commits into from
May 22, 2021

Conversation

taiki-e
Copy link
Collaborator

@taiki-e taiki-e commented May 22, 2021

No description provided.

@taiki-e taiki-e merged commit c3c5f2a into master May 22, 2021
@taiki-e taiki-e deleted the taiki-e/named branch May 22, 2021 12:48
Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

Neat!

@taiki-e taiki-e mentioned this pull request May 22, 2021
@Kestrer
Copy link

Kestrer commented May 22, 2021

What happens if the Readable/Writable futures outlive the Async? Could that cause problems?

@taiki-e
Copy link
Collaborator Author

taiki-e commented May 22, 2021

Hmm...

Error: Os { code: 9, kind: Other, message: "Bad file descriptor" }

@taiki-e
Copy link
Collaborator Author

taiki-e commented May 22, 2021

In this case, the error is expected behavior, but I think I should have left a reference to self to prevent users from writing such code...
async-net's TcpStream and UnixStream wraps the Async in Arc, so it would have made more sense to add a method like readable_owned that takes self: &Arc<Self> instead...

1.5.0 has only been released about 2 hours ago, and I think the only thing that depends on this feature is async-net's master, so I'll yank 1.5.0...

@Kestrer Thanks for catching this problem.

@Kestrer
Copy link

Kestrer commented May 22, 2021

Oh no, is there no way to support this? Would it be possible to for example perform a check if the resource has been unregistered and have Readable error out?

@taiki-e
Copy link
Collaborator Author

taiki-e commented May 22, 2021

@Kestrer

Would it be possible to for example perform a check if the resource has been unregistered and have Readable error out?

I think it is possible, but I would like to provide an API that prevents such error if possible.

In fact, the error that occurs, in that case, can be easily reproduced by more low-level libraries such as polling, so this PR's API is not necessarily bad. So, we could simply add documentation that says "If writable/readable polled after Async dropped, it will result in an error", but I tend to think it is better if this could be prevented at compile time...

(I'm still not convinced it's worth the breaking change, but we found the problem fast enough that I didn't have to worry about the impact of the breaking change, so I yanked 1.5.0 anyway. Sorry if anyone else is affected by this yanking!)

@taiki-e
Copy link
Collaborator Author

taiki-e commented May 22, 2021

And here is the API & implementation I'm thinking of: master...taiki-e/owned
(TR; DR: change Readable and Writable to contain a reference to Async, and add Async::{readable_owned, writable_owned} that take self: &Arc<Self>)

I'm not sure about other use cases where this feature was needed, but it is enough for the async-net use case (smol-rs/async-net@master...taiki-e/owned),

@Kestrer @smol-rs/admins Any thoughts on this?

@Kestrer
Copy link

Kestrer commented May 22, 2021

I think this is a good idea. {readable, writable}_owned should probably take Arc<Self> to reduce unnecessary cloning. And it might be cleaner to implement it using a generic AsRef<Source>, implemented by both Arc<Async> and &Async.

@taiki-e
Copy link
Collaborator Author

taiki-e commented May 23, 2021

Filed #66. Thanks @Kestrer for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants