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 Stream and Sink future traits to IpcSender and Receiver #165

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

illegalprime
Copy link

It would be cool if this could work with async futures-rs. I think this will make servo/servo#16236 a lot easier.

So far Stream for IpcReceiver was pretty straightforward, but I'm wondering about IpcSender. Does anyone have any tips on how to move forward with the sender? I need to implemented start_send and poll_complete and I'm not sure how to logically break up the sending code into those two methods.

@highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

@jdm
Copy link
Member

jdm commented Jun 1, 2017

It's not clear to me that it's possible to break up sending into separate steps, since the per-platform implementations all invoke operations that don't have any kind of blocking semantics as far as I can tell.

@illegalprime
Copy link
Author

@jdm does that mean each call to start_send should just complete the send operation?

@jdm
Copy link
Member

jdm commented Jun 1, 2017

I believe so.

@illegalprime
Copy link
Author

OK, thanks for the tip, I'll finish that up and update the PR

@antrik
Copy link
Contributor

antrik commented Jun 1, 2017

To clarify: the send() method currently doesn't provide any async semantics -- but the call definitely can block in certain situations.

For one, there is a limit on how many unreceived messages can be pending on a channel, before sending another one blocks. (With the unix back-end, only one unreceived message can be pending at any time; on macos, it depends on what Mach defaults to; and on the upcoming windows one in #108 , I think it depends on the size of the messages...)

What's more, large sends on the unix back-end trigger automatic fragmentation, and can always block in the middle of the operation -- unless the receiver side is very quick -- even if no other messages were pending. (I think also on windows -- though I don't remember exactly right now how the fragmentation works there.)

I'm not sure a simplistic Stream implementation for the sender, that can actually block indefinitely, would be a good idea: that's probably not something the users expect... To implement this properly, we will need to extend the platform back-ends for the send() implementation, to enable actual async operation.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #164) made this pull request unmergeable. Please resolve the merge conflicts.

@illegalprime
Copy link
Author

@antrik OK I can try, help would be great though. Is the Stream implementation good enough for the Receiver?

@antrik
Copy link
Contributor

antrik commented Jun 20, 2017

@illegalprime yeah, the Receiver implementation looks good from what I remember :-)

I'd be glad to help -- what form of help where you thinking of specifically?

@spinda
Copy link
Contributor

spinda commented Aug 21, 2017

Could the Receiver part of this be merged for the time being, since Sender part seems to have stalled out? I'd be happy to help with that if necessary. For Sender, though, the platform-specific changes required are probably beyond me right now.

@jdm
Copy link
Member

jdm commented Aug 21, 2017

I would merge a PR that contained the Receiver changes.

@jdm
Copy link
Member

jdm commented Aug 21, 2017

Well, I'd probably ask that it be placed behind a cargo feature.

spinda added a commit to spinda/ipc-channel that referenced this pull request Aug 21, 2017
Extracts the IpcReceiver half of servo#165.

This is placed behind an `async` feature so the dependency on futures isn't
forced.
bors-servo pushed a commit that referenced this pull request Aug 21, 2017
Make IpcReceiver a futures::Stream

Extracts the IpcReceiver half of #165.

This is placed behind an `async` feature so the dependency on futures isn't forced.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants