Added unit test for reentrancy. #95

asajeffrey · 2016-08-09T18:19:56Z

IPC channels are reentrant, in that if we call c.send(a) and serializing a calls c.send(b), then the channel contents are b then a. (This comes up in the case of logging over channels, since serialization may include logging.)

This PR adds a unit test to make sure we don't break reentrancy.

Discussion with @nox on IRC.

asajeffrey · 2016-08-09T18:20:13Z

cc @pcwalton

antrik · 2016-08-09T18:59:41Z

src/test.rs

+#[derive(Clone, Debug, Eq, PartialEq)]
+struct HasWeirdSerializer (Option<String>);
+
+lazy_static! { static ref WEIRD_CHANNEL: (IpcSender<HasWeirdSerializer>, IpcReceiver<HasWeirdSerializer>) = ipc::channel().unwrap(); }


A non-reentrant testcase for reentrance? Now that's just lazy! ;-)

I guess it's not really a problem, unless the test harness ever grows an ability to run the same test multiple times in parallel, which seems rather hypothetical...

antrik · 2016-08-09T19:05:16Z

For what it's worth, this looks good to me.

(I'm happy to see a testcase being added for obscure behaviour someone will be relying on -- a very good practice that probably should be employed more often :-) )

jdm · 2016-08-09T22:12:51Z

Travis is displeased:

     Running `rustc src/lib.rs --crate-name ipc_channel -g --test -C metadata=f57b0507e715624e -C extra-filename=-f57b0507e715624e --out-dir /Users/travis/build/servo/ipc-channel/target/debug/deps --emit=dep-info,link -L dependency=/Users/travis/build/servo/ipc-channel/target/debug/deps --extern rand=/Users/travis/build/servo/ipc-channel/target/debug/deps/librand-49a08859d086fffe.rlib --extern lazy_static=/Users/travis/build/servo/ipc-channel/target/debug/deps/liblazy_static-359f5533c970cd71.rlib --extern crossbeam=/Users/travis/build/servo/ipc-channel/target/debug/deps/libcrossbeam-95e5cb210400af41.rlib --extern bincode=/Users/travis/build/servo/ipc-channel/target/debug/deps/libbincode-6bdca1673f724e3d.rlib --extern libc=/Users/travis/build/servo/ipc-channel/target/debug/deps/liblibc-1bd8847afb79f283.rlib --extern serde=/Users/travis/build/servo/ipc-channel/target/debug/deps/libserde-652fd324488db42d.rlib`

<lazy_static macros>:2:32: 3:65 error: the trait bound `std::cell::Cell<u32>: std::marker::Sync` is not satisfied [E0277]

<lazy_static macros>:2 use std :: sync :: ONCE_INIT ; static mut $ NAME : $ crate :: lazy :: Lazy < $

                                                      ^

<lazy_static macros>:21:1: 21:40 note: in this expansion of __lazy_static_create! (defined in <lazy_static macros>)

<lazy_static macros>:4:1: 5:75 note: in this expansion of lazy_static! (defined in <lazy_static macros>)

src/test.rs:56:1: 56:135 note: in this expansion of lazy_static! (defined in <lazy_static macros>)

<lazy_static macros>:2:32: 3:65 help: run `rustc --explain E0277` to see a detailed explanation

<lazy_static macros>:2:32: 3:65 note: `std::cell::Cell<u32>` cannot be shared between threads safely 

<lazy_static macros>:2:32: 3:65 note: required because it appears within the type `platform::os::OsIpcReceiver` 

<lazy_static macros>:2:32: 3:65 note: required because it appears within the type `ipc::IpcReceiver<test::HasWeirdSerializer>` 

<lazy_static macros>:2:32: 3:65 note: required because it appears within the type `(ipc::IpcSender<test::HasWeirdSerializer>, ipc::IpcReceiver<test::HasWeirdSerializer>)` 

<lazy_static macros>:2:32: 3:65 note: required by `lazy_static::lazy::Lazy` 

error: aborting due to previous error 

error: Could not compile `ipc-channel`.

asajeffrey · 2016-08-10T04:03:53Z

Not sure why I'm not getting this error on my end... I'll put it in a rw mutex or some such and retry.

antrik · 2016-08-10T05:11:26Z

@asajeffrey on what platform(s) have you tested this? The definition of IpcReceiver is presently kinda wrong on the linux and inprocess backends (only the macos one gets it right): the definitions formally look thread-safe, which is not really true -- see discussion on #89 . I have some work in progress to fix this, which I haven't quite wrapped up yet; but once that's in, your code would fail equally on all platforms.

asajeffrey · 2016-08-10T05:36:38Z

The test isn't to see if it's thread-safe, it's to see if it's reentrant. Since IPC channels serialize to a fresh buffer each time, it should be reentrant.

antrik · 2016-08-10T05:57:38Z

@asajeffrey well, that's not what the test is about -- but since lazy_static creates a global variable, Rust enforces that whatever it holds must be thread-safe. That's not really the case for IpcReceiver.

asajeffrey · 2016-08-10T16:21:38Z

Indeed, since it's a global it has to be sync. Rather ironically this is exactly the use case for reentrant locks, which is what got is here in the first place. Probably easiest just to make it thread-local. I'm not at my desk today, I'll do this tomorrow.

asajeffrey · 2016-08-11T00:11:27Z

I made Travis happy by making the global thread-local.

antrik · 2016-08-13T18:26:17Z

src/test.rs

+        Ok(HasWeirdSerializer(try!(Deserialize::deserialize(deserializer))))
+    }
+}
+


Personally I think it would be more readable to put all this stuff right before the (only) test case using it...

Fair enough.

antrik · 2016-08-13T18:32:37Z

src/test.rs

+    let hello = HasWeirdSerializer(Some(String::from("hello")));
+    let (sender, receiver) = ipc::channel().unwrap();
+    let sender1 = sender.clone();
+    WEIRD_CHANNEL.with(|chan| { *chan.borrow_mut() = Some(sender1); });


Nitpick: since this is not a move closure, it seem circuitous to do the clone() beforehand -- I think it would be cleaner simply to do Some(sender.clone()) within the closure...

This is mostly just a general hint though -- as we are talking about test code here, I wouldn't insist on fixing such a minor stylistic wrinkle :-)

Okay, I'll make this change.

antrik · 2016-08-15T19:18:21Z

@asajeffrey thanks :-) Please squash the commits -- and then we just need someone with official reviewer status to approve it...

asajeffrey · 2016-08-15T20:43:24Z

Squashed.

antrik · 2016-08-15T21:52:24Z

Meh, there is an intermittent failure it seems? This definitely shouldn't be affected by these changes...

asajeffrey · 2016-08-15T22:12:46Z

Yes, that is odd... timeouts on unrelated tests?

antrik · 2016-08-20T15:21:52Z

Just to be clear: I really don't think this test failure has anything to do with this PR. Either the test case in question has intermittent failures, or there was some one-time hiccup on Travis.

@pcwalton r?

bors-servo · 2016-08-29T08:43:18Z

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

pcwalton · 2016-08-29T19:19:15Z

Looks good once the conflicts are resolved.


        Added unit test for reentrancy.

asajeffrey · 2016-09-05T14:10:56Z

@bors-servo r=pcwalton

bors-servo · 2016-09-05T14:10:57Z

📌 Commit 3a06775 has been approved by pcwalton

bors-servo · 2016-09-05T14:10:58Z

⌛ Testing commit 3a06775 with merge ca63fdb...


        Auto merge of #95 - asajeffrey:master, r=pcwalton

@nox

Added unit test for reentrancy. IPC channels are reentrant, in that if we call `c.send(a)` and serializing `a` calls `c.send(b)`, then the channel contents are `b` then `a`. (This comes up in the case of logging over channels, since serialization may include logging.) This PR adds a unit test to make sure we don't break reentrancy. Discussion with @nox on IRC.

bors-servo · 2016-09-05T14:21:44Z

☀️ Test successful - status-appveyor, status-travis

antrik reviewed Aug 9, 2016
View changes

antrik reviewed Aug 13, 2016
View changes

asajeffrey force-pushed the asajeffrey:master branch from 64c177c to 34123ea Aug 15, 2016

asajeffrey force-pushed the asajeffrey:master branch 2 times, most recently from 36af002 to 525f6d6 Sep 5, 2016

Added unit test for reentrancy.

Loading status checks…

3a06775

asajeffrey force-pushed the asajeffrey:master branch from 525f6d6 to 3a06775 Sep 5, 2016

bors-servo merged commit 3a06775 into servo:master Sep 5, 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

servo / ipc-channel

Added unit test for reentrancy. #95

Added unit test for reentrancy. #95

asajeffrey commented Aug 9, 2016

asajeffrey commented Aug 9, 2016

This comment has been minimized.

antrik commented Aug 9, 2016

jdm commented Aug 9, 2016

asajeffrey commented Aug 10, 2016

antrik commented Aug 10, 2016

asajeffrey commented Aug 10, 2016

antrik commented Aug 10, 2016

asajeffrey commented Aug 10, 2016

asajeffrey commented Aug 11, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

antrik commented Aug 15, 2016

asajeffrey commented Aug 15, 2016

antrik commented Aug 15, 2016

asajeffrey commented Aug 15, 2016

antrik commented Aug 20, 2016

bors-servo commented Aug 29, 2016

pcwalton commented Aug 29, 2016

asajeffrey commented Sep 5, 2016

bors-servo commented Sep 5, 2016

bors-servo commented Sep 5, 2016

bors-servo commented Sep 5, 2016

servo / ipc-channel

Join GitHub today

Added unit test for reentrancy. #95

Added unit test for reentrancy. #95

Conversation

asajeffrey commented Aug 9, 2016

asajeffrey commented Aug 9, 2016

This comment has been minimized.

antrik Aug 9, 2016 • edited

antrik commented Aug 9, 2016

jdm commented Aug 9, 2016

asajeffrey commented Aug 10, 2016

antrik commented Aug 10, 2016

asajeffrey commented Aug 10, 2016

antrik commented Aug 10, 2016

asajeffrey commented Aug 10, 2016

asajeffrey commented Aug 11, 2016

This comment has been minimized.

antrik Aug 13, 2016

This comment has been minimized.

asajeffrey Aug 13, 2016

This comment has been minimized.

antrik Aug 13, 2016

This comment has been minimized.

asajeffrey Aug 13, 2016

antrik commented Aug 15, 2016

asajeffrey commented Aug 15, 2016

antrik commented Aug 15, 2016

asajeffrey commented Aug 15, 2016

antrik commented Aug 20, 2016

bors-servo commented Aug 29, 2016

pcwalton commented Aug 29, 2016

asajeffrey commented Sep 5, 2016

bors-servo commented Sep 5, 2016

bors-servo commented Sep 5, 2016

bors-servo commented Sep 5, 2016

antrik Aug 9, 2016 •

edited