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

Introduce force-inprocess feature #92

Merged
merged 3 commits into from Aug 29, 2016
Merged

Introduce force-inprocess feature #92

merged 3 commits into from Aug 29, 2016
+39 −24

Conversation

@nox
Copy link
Member

nox commented Aug 2, 2016

No description provided.

@nox nox mentioned this pull request Aug 2, 2016
Merged
@bors-servo
Copy link
Contributor

bors-servo commented Aug 3, 2016

☔ The latest upstream changes (presumably #93) made this pull request unmergeable. Please resolve the merge conflicts.
@antrik
Copy link
Contributor

antrik commented Aug 10, 2016

What's up with this? Does the test failure point to an actual problem, or was that just some kind of Travis hiccup?...
@nox
Copy link
Member Author

nox commented Aug 10, 2016

@antrik Sounds like an actual failure:

fatal runtime error: out of memory
fatal runtime error: out of memory
fatal runtime error: out of memory
fatal runtime error: out of memory
fatal runtime error: out of memory
fatal runtime error: out of memory
fatal runtime error: out of memory
fatal runtime error: out of memory

Will rebase and retry.
@nox nox force-pushed the nox:inprocess branch from 66dc17f to d7122c5 Aug 20, 2016
@antrik
Copy link
Contributor

antrik commented Aug 20, 2016 •
edited

@nox oh, I see what happened there: the condition for the fragmenting tests at https://github.com/servo/ipc-channel/blob/master/src/platform/test.rs#L207 needs to be adapted too, so these tests only run when actually using the linux backend. They make no sense -- and won't work -- with backends not using fragmentation.

BTW, I wonder whether there is some way to determine the backend to use in a single place, and store this in some kind of config variable that is used throughout the library, rather than having system-specific conditionals everywhere? (If there isn't, I'd consider that a serious shortcoming in Rust/Cargo...)
@nox
Copy link
Member Author

nox commented Aug 20, 2016

@antrik Nice debugging, will disable these tests.
@nox nox force-pushed the nox:inprocess branch from d7122c5 to dac0dce Aug 24, 2016
src/test.rs Outdated
@@ -130,8 +129,7 @@ fn select() {
}

#[test]
///XXXjdm Windows' libc doesn't include fork.

This comment has been minimized.

Sign in to view
@Manishearth

Manishearth Aug 24, 2016

Member

Keep the comment? r=me either way

This comment has been minimized.

Sign in to view
@antrik

antrik Aug 24, 2016

Contributor

I think keeping the comment would be confusing: the multi-process tests actually can't work / don't make sense on platforms using the inprocess implementation -- regardless of whether fork() is available or not.
@nox nox force-pushed the nox:inprocess branch from dac0dce to 11615c3 Aug 24, 2016
@nox
Copy link
Member Author

nox commented Aug 24, 2016

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Aug 24, 2016

📌 Commit 11615c3 has been approved by nox
@bors-servo
Copy link
Contributor

bors-servo commented Aug 24, 2016

⌛ Testing commit 11615c3 with merge 055514f...
bors-servo added a commit that referenced this pull request Aug 24, 2016
@bors-servo
Auto merge of #92 - nox:inprocess, r=nox
Loading status checks…
055514f 
Introduce force-inprocess feature
@nox
Copy link
Member Author

nox commented Aug 24, 2016

@bors-servo r-
@nox
Copy link
Member Author

nox commented Aug 24, 2016

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

bors-servo commented Aug 24, 2016

📌 Commit 11615c3 has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Aug 24, 2016

💔 Test failed - status-travis
@nox nox force-pushed the nox:inprocess branch from 11615c3 to 02c7e75 Aug 24, 2016
@nox
Copy link
Member Author

nox commented Aug 24, 2016

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

bors-servo commented Aug 24, 2016

📌 Commit 02c7e75 has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Aug 24, 2016

⌛ Testing commit 02c7e75 with merge 345ff2e...
bors-servo added a commit that referenced this pull request Aug 24, 2016
@bors-servo
Auto merge of #92 - nox:inprocess, r=Manishearth
Loading status checks…
345ff2e 
Introduce force-inprocess feature
@bors-servo
Copy link
Contributor

bors-servo commented Aug 24, 2016

💔 Test failed - status-travis
@nox
Copy link
Member Author

nox commented Aug 24, 2016

@antrik Not running that test wasn't enough.
src/platform/test.rs Outdated
let (mut received_data, received_channels, received_shared_memory_regions) =
super_rx.recv().unwrap();
// These tests only apply to platforms that need fragmentation.
#[cfg(all(not(feature = "inprocess"), target_os = "linux"))]

This comment has been minimized.

Sign in to view
@antrik

antrik Aug 24, 2016

Contributor

It's force-inprocess, not just inprocess.

This comment has been minimized.

Sign in to view
@nox

nox Aug 25, 2016

Author Member

Oh, I'm silly, thanks.
src/platform/test.rs Outdated
assert_eq!(&received_data[..], &data[..]);
assert_eq!(received_channels.len(), receivers.len());
assert_eq!(received_shared_memory_regions.len(), 0);
fn with_n_fds(n: usize, size: usize) {

This comment has been minimized.

Sign in to view
@antrik

antrik Aug 24, 2016

Contributor

I actually left the function outside of the conditional block on purpose, as it's not really platform-specific, and some existing and/or newly added generic tests could start using it too... I don't feel strongly about this though.
nox added 3 commits Aug 2, 2016
@nox
Introduce force-inprocess feature
3e9aee8
@nox
Fix all import warnings
e7519f0
@nox
Make inprocess::OsIpcSender::to_sender take &mut self
Loading status checks…
4d0369c 
The other implementations take that, and the implementations should
behave consistently.
@nox nox force-pushed the nox:inprocess branch from 02c7e75 to 4d0369c Aug 25, 2016
@antrik
Copy link
Contributor

antrik commented Aug 26, 2016

Looks good :-) As you added the warning clean-ups, I guess it needs a new r+? Or is the previous one sufficient to cover this?...
@nox
Copy link
Member Author

nox commented Aug 26, 2016

Just wanted you to take a last look. :)

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

bors-servo commented Aug 26, 2016

📌 Commit 4d0369c has been approved by antrik
@bors-servo
Copy link
Contributor

bors-servo commented Aug 26, 2016

⌛ Testing commit 4d0369c with merge b48a8d3...
bors-servo added a commit that referenced this pull request Aug 26, 2016
@bors-servo
Auto merge of #92 - nox:inprocess, r=antrik
Loading status checks…
b48a8d3 
Introduce force-inprocess feature
@bors-servo
Copy link
Contributor

bors-servo commented Aug 26, 2016

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

nox commented Aug 29, 2016

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Aug 29, 2016

⌛ Testing commit 4d0369c with merge 3852245...
bors-servo added a commit that referenced this pull request Aug 29, 2016
@bors-servo
Auto merge of #92 - nox:inprocess, r=antrik
Loading status checks…
3852245 
Introduce force-inprocess feature
@bors-servo
Copy link
Contributor

bors-servo commented Aug 29, 2016

☀️ Test successful - status-appveyor, status-travis
@bors-servo bors-servo merged commit 4d0369c into servo:master Aug 29, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
This was referenced Aug 29, 2016
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@nox @bors-servo @antrik @Manishearth
You can’t perform that action at this time.