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

Fix IpcOneShotServer for `inprocess` backend #90

Closed
wants to merge 3 commits into from
Closed

Fix IpcOneShotServer for `inprocess` backend #90

wants to merge 3 commits into from

Conversation

@antrik
Copy link
Contributor

antrik commented Aug 1, 2016

Several patches covering individual aspects, each fixing one possible situation that was broken.

(I don't think there was actually any situation that was working correctly before...)
@antrik
Copy link
Contributor Author

antrik commented Aug 1, 2016

I tested this by forcing the inprocess backend on GNU/Linux -- let's see whether it actually works on Windows as well...
@jdm
Copy link
Member

jdm commented Aug 1, 2016 

---- platform::test::server_no_accept stdout ----

    thread 'platform::test::server_no_accept' panicked at 'assertion failed: pingback_rx.try_recv().is_err()', platform/test.rs:426

note: Run with `RUST_BACKTRACE=1` for a backtrace.

failures:

    platform::test::server_no_accept
@jdm
Copy link
Member

jdm commented Aug 1, 2016

Looks like the test failure happens on mac.
@antrik
Copy link
Contributor Author

antrik commented Aug 1, 2016

Actually, I am surprised that it didn't fail on the GNU/Linux builder: it turns out this test case fails on my system when using the Linux backend. (I forgot to try that before...)

It seems I made a wrong assumption about how connect() behaves on sockets, and consequently how it's supposed to behave in ipc-channel. I'll rework the series to account for that.
@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 added 3 commits Aug 1, 2016
@antrik
platform/inprocess: Avoid deadlock by cloning `ServerRecord`
0aba972 
Holding a global lock on the `ONE_SHOT_SERVERS` map while doing the
blocking `accept()` invocation is a very bad idea -- aside from possible
long-range concurrency problems or deadlocks, it actually creates a race
condition between `accept()` and `connect()`, which the existing test
case triggers pretty much every time on my system.

The solution used here is not the most elegant IMHO -- but it's the only
one I can think of that preserves the current abstraction, with
`accept()` and `connect()` methods on `ServerRecord`.

Possible more elegant (but also way more invasive) solutions would be
moving the `conn_receiver` (along with the `accept()` implementation) to
`OsIpcOneShotServer`; or even keeping *only* `conn_receiver` in
`OsIpcOneShotServer`, while the actual data channel is only created and
sent over the connect channel in the connect/accept phase -- thus
bringing it more in line with the socket implementation...
@antrik
tests: Enable `server` test on Windows
4177413 
With the deadlock in the `inprocess` implementation fixed, this should
work fine on Windows now.
@antrik
platform/inprocess: Drop server record after `accept()`, not on `conn…
Loading status checks…
40b42a4 
…ect()`

Invoking `accept()` after `connect()` used to fail, because the server
record was being dropped while doing the `connect()`. Now dropping it
after the `accept()` instead -- which is safe for either invocation
order, because `accept()` is blocking, and thus never finishes before
`connect()` is done.

Also adding a separate `server_connect_first` test case to explicitly
test this situation. (And modifying the original test case to ensure it
always tests the other situation.)
@antrik antrik force-pushed the antrik:fix-inprocess-oneshotserver branch from 530b99b to 40b42a4 Aug 13, 2016
@antrik
Copy link
Contributor Author

antrik commented Aug 13, 2016

Looks better now.

I also implemented an alternate approach, using a Drop trait for cleanup: antrik@3d1e140 I'd say it is more elegant -- but I'm not entirely sure it's a good idea, because of the locking involved... Opinions?
@bors-servo
Copy link
Contributor

bors-servo commented Aug 29, 2016

☔ The latest upstream changes (presumably #92) made this pull request unmergeable. Please resolve the merge conflicts.
@pcwalton
Copy link
Collaborator

pcwalton commented Aug 29, 2016

Looks good once conflicts are resolved. Sorry about the delay!
@emilio emilio mentioned this pull request Aug 31, 2016
Merged
bors-servo added a commit that referenced this pull request Aug 31, 2016
@bors-servo
Auto merge of #100 - emilio:fix-inprocess-oneshot, r=pcwalton
Loading status checks…
cddb6a8 
Fix inprocess oneshot

Rebase of #90.

@antrik sent a mail to the list saying he was not going to be active, so I took care of rebasing this.
@emilio
Copy link
Member

emilio commented Aug 31, 2016

Rebased in #100,

Thanks, @antrik! :)
@emilio emilio closed this Aug 31, 2016
bors-servo added a commit that referenced this pull request Aug 31, 2016
@bors-servo
Auto merge of #100 - emilio:fix-inprocess-oneshot, r=emilio,pcwalton
Loading status checks…
65d8404 
Fix inprocess oneshot

Rebase of #90.

@antrik sent a mail to the list saying he was not going to be active, so I took care of rebasing this.
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

5 participants
@antrik @jdm @bors-servo @pcwalton @emilio
You can’t perform that action at this time.