Skip to content

/ ipc-channel

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

Support futures 0.3 streams for IPC channels #227

Merged
merged 3 commits into from May 6, 2019
Merged

Support futures 0.3 streams for IPC channels #227

merged 3 commits into from May 6, 2019
+139 −56

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Mar 4, 2019

Fixes #219
@asajeffrey
Adding a router from IPC channels to future channels, which allows th…
Loading status checks…
c35879d 
…em to be used in async settings
@asajeffrey
Version bump
Loading status checks…
2592931
@asajeffrey
Copy link
Member Author

asajeffrey commented Mar 4, 2019

This is a breaking change to the API, so needs a bump to 0.12.

The implementation uses OpaqueIpcReceiver, which isn't supported by IpcBytesReceiver, so I couldn't see how to implement Stream for it.

Since futures 0.3 is still only on nightly, this PR means that async is now only on nightly. We could try to maintain both, but that seems difficult.
This was referenced Mar 4, 2019
Closed
Closed
@asajeffrey
Copy link
Member Author

asajeffrey commented Mar 4, 2019

r? @antrik
@asajeffrey asajeffrey mentioned this pull request Mar 4, 2019
Open
@asajeffrey
Copy link
Member Author

asajeffrey commented Mar 11, 2019

Ping? @antrik
@dlrobertson
dlrobertson reviewed Mar 31, 2019
View changes
src/async.rs Outdated
impl<T> Unpin for IpcStream<T> {}

// A router which routes from an IPC channel to a stream.
struct Router {

This comment has been minimized.

Sign in to view
@dlrobertson

dlrobertson Mar 31, 2019

Collaborator

Router and ROUTER are used in router.rs. Would it make sense to rename these to AsyncRouter and ASYNC_ROUTER?

This comment has been minimized.

Sign in to view
@asajeffrey

asajeffrey Apr 1, 2019

Author Member

It's AsyncRouter vs async::Router which I don't have strong opinions about.
@asajeffrey
Copy link
Member Author

asajeffrey commented May 6, 2019

Review ping @antrik or @dlrobertson?
@dlrobertson
dlrobertson reviewed May 6, 2019
View changes
Copy link
Collaborator

dlrobertson left a comment

Looks good to me, but I'm not as familiar with futures or the ROUTER.
@asajeffrey
Copy link
Member Author

asajeffrey commented May 6, 2019

@bors-servo r=dlrobertson
@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2019

📌 Commit 2592931 has been approved by dlrobertson
@highfive highfive assigned dlrobertson and unassigned jdm May 6, 2019
@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2019

⌛ Testing commit 2592931 with merge f9de331...
bors-servo added a commit that referenced this pull request May 6, 2019
@bors-servo
Auto merge of #227 - asajeffrey:futures-0.3, r=dlrobertson
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
f9de331 
Support futures 0.3 streams for IPC channels

Fixes #219
@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2019

💔 Test failed - status-appveyor
@asajeffrey
Copy link
Member Author

asajeffrey commented May 6, 2019

Argh, async is a keyword these days.

81 | pub mod async;
   |         ^^^^^ expected identifier, found reserved keyword
help: you can escape reserved keywords to use them as identifiers
   |
81 | pub mod r#async;
   |         ^^^^^^^
@asajeffrey
Async is a keyword, so renaming the module to asynch
Loading status checks…
5cf299d
@asajeffrey
Copy link
Member Author

asajeffrey commented May 6, 2019

@bors-servo r=dlrobertson
@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2019

📌 Commit 5cf299d has been approved by dlrobertson
@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2019

⌛ Testing commit 5cf299d with merge 7cc64eb...
bors-servo added a commit that referenced this pull request May 6, 2019
@bors-servo
Auto merge of #227 - asajeffrey:futures-0.3, r=dlrobertson
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
7cc64eb 
Support futures 0.3 streams for IPC channels

Fixes #219
@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: dlrobertson
Pushing 7cc64eb to master...
@bors-servo bors-servo merged commit 5cf299d into servo:master May 6, 2019
3 checks passed
3 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlrobertson dlrobertson

Assignees

@dlrobertson dlrobertson

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Update to futures 0.3
5 participants
@asajeffrey @bors-servo @dlrobertson @jdm @highfive
You can’t perform that action at this time.