Implement ipc-channel on Windows #108

vvuk · 2016-10-05T01:11:08Z

This implementation uses named pipes on Windows, with auto-generated uuid
names. It takes advantage of DuplicateHandle to clone handles to target
processes when sending handles cross-process. Shared memory is implemented
using anonymous file mappings. select() and friends are implemented using
IO Completion Ports. I learned a lot about Windows IO doing this, more than I wanted to :)

All the current tests pass. I have not tried actually running servo with multiprocess ipc -- that's next, but I wanted to get this up for review first. There's a branch (win32-unsquashed) that has a large sequence of commits that led to this, in case that's interesting to anyone... it contains a couple of abandoned approaches early on though.

antrik · 2016-10-05T02:57:26Z

Cargo.toml

+[target.'cfg(target_os = "windows")'.dependencies]
+winapi = "*"
+user32-sys = "*"
+kernel32-sys = "*"


Uh... Aren't wildcard dependencies considered evil?

Yeah, remove the wildcard dependencies. crates.io will reject them.

antrik · 2016-10-05T03:02:24Z

Great work!

Before starting a proper review, I'd ask you to actually test Servo first though, to avoid extra effort in case further changes turn out necessary...

BTW, you could also try running the benchmarks. While these are not really intended to test functionality, the extra coverage can't hurt -- and it would actually be interesting to see how this implementation stacks up against the others... You could in fact include the results in the commit message, so we get an idea as well :-)

vvuk · 2016-10-06T00:35:33Z

Ok, I'll admit, I forgot that 32-bit was a thing that existed. I'll fix that up D:

Servo worked first time around in non-multiprocess mode (using the Windows channels). In multiprocess mode (with modifications to constellation that I'm going to put in a PR), I get a panic about some subchannel data that I'm going to debug. But I think we can start reviews and even landing, and keep non-multiprocess as the default until that's tracked down in parallel.

Benchmark results below -- I don't have linux/OSX running on the same hardware so I can't compare directly, but this is on a Core i7-4770K. I haven't done any performance work or even profiling on it, so there's probably some easy wins.

running 25 tests
test create_channel ... bench:      18,590 ns/iter (+/- 1,422)
test size_00_1      ... bench:       2,941 ns/iter (+/- 136)
test size_01_2      ... bench:       2,943 ns/iter (+/- 186)
test size_02_4      ... bench:       2,936 ns/iter (+/- 141)
test size_03_8      ... bench:       2,955 ns/iter (+/- 143)
test size_04_16     ... bench:       2,952 ns/iter (+/- 222)
test size_05_32     ... bench:       2,936 ns/iter (+/- 206)
test size_06_64     ... bench:       2,958 ns/iter (+/- 156)
test size_07_128    ... bench:       2,959 ns/iter (+/- 163)
test size_08_256    ... bench:       2,991 ns/iter (+/- 143)
test size_09_512    ... bench:       3,089 ns/iter (+/- 227)
test size_10_1k     ... bench:       3,175 ns/iter (+/- 166)
test size_11_2k     ... bench:       6,150 ns/iter (+/- 281)
test size_12_4k     ... bench:       6,563 ns/iter (+/- 414)
test size_13_8k     ... bench:       5,891 ns/iter (+/- 236)
test size_14_16k    ... bench:       7,478 ns/iter (+/- 243)
test size_15_32k    ... bench:      18,362 ns/iter (+/- 1,764)
test size_16_64k    ... bench:      26,334 ns/iter (+/- 1,614)
test size_17_128k   ... bench:     214,230 ns/iter (+/- 11,577)
test size_18_256k   ... bench:     261,279 ns/iter (+/- 11,749)
test size_19_512k   ... bench:     628,903 ns/iter (+/- 134,958)
test size_20_1m     ... bench:   1,630,359 ns/iter (+/- 245,115)
test size_21_2m     ... bench:   3,473,931 ns/iter (+/- 245,493)
test size_22_4m     ... bench:   7,568,551 ns/iter (+/- 406,633)
test size_23_8m     ... bench:  15,618,413 ns/iter (+/- 725,515)

vvuk · 2016-10-12T01:14:39Z

review ping... @pcwalton @antrik

antrik · 2016-10-13T13:28:14Z

@vvuk it's on my list... Sorry for being slow :-(

vvuk · 2016-10-13T14:26:45Z

No problem! Just making sure it wasn't blocked on anything.

On Oct 13, 2016 9:28 AM, "Olaf Buddenhagen" notifications@github.com
wrote:

@vvuk https://github.com/vvuk it's on my list... Sorry for being slow
:-(

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#108 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAL5lVaNNYMIQPTx_LtVJ0xWKzWn8Xvdks5qzjHvgaJpZM4KOU0X
.

pcwalton · 2016-10-13T21:57:29Z

src/platform/windows/mod.rs

+    static ref DD_ENABLED: bool = match env::var_os("DD") {
+        Some(_) => true,
+        None => false,
+    };


This can just be env::var_os("DD").is_some()

pcwalton · 2016-10-13T21:57:34Z

src/platform/windows/mod.rs

+    static ref DD2_ENABLED: bool = match env::var_os("DD2") {
+        Some(_) => true,
+        None => false,
+    };


pcwalton

I'm not an IOCP expert so I'll defer to @antrik for that, but I just had a few nits.

Very glad to see that this actually works in Win32 :)

pcwalton · 2016-10-13T21:58:04Z

src/platform/windows/mod.rs

+// When we create the pipe, how big of a write buffer do we specify?
+// This is reserved in the nonpaged pool.
+//
+// This is 80k because a lot of the test assume that we can write a


uber-nit: "tests"

pcwalton · 2016-10-13T22:00:06Z

src/platform/windows/mod.rs

+
+        // if the remote end closed...
+        if err != winapi::ERROR_SUCCESS {
+            panic!("[$ {:?}:{:?}] *** notify_completion: need to handle error! {}", self.iocp, self.handle, err);


I guess add a FIXME here?

pcwalton · 2016-10-13T22:01:19Z

src/platform/windows/mod.rs

+            let more =
+                if buf_cap == 0 { READ_BUFFER_SIZE }
+            else if buf_cap < READ_BUFFER_MAX_GROWTH { buf_cap }
+            else { READ_BUFFER_MAX_GROWTH };


Could be:

match buf_cap { 0 => READ_BUFFER_SIZE, 1...READ_BUFFER_MAX_GROWTH => buf_cap, _ => READ_BUFFER_MAX_GROWTH, }

pcwalton · 2016-10-13T22:01:53Z

src/platform/windows/mod.rs

+
+    // Err(false) -> something really failed
+    // Err(true) -> no message
+    // XXX This is dumb, we should return


nit: use FIXME

pcwalton · 2016-10-13T22:03:24Z

src/platform/windows/mod.rs

+        return Ok(());
+    }
+    let mut nwritten: u32 = 0;
+    //dd!("[c {:?}] writing: {:?}", handle, bytes);


Uncomment or remove

pcwalton · 2016-10-13T22:03:29Z

src/platform/windows/mod.rs

+        }
+        nwritten += nwrote;
+        ntowrite -= nwrote;
+        //dd!("[c {:?}] ... wrote {} bytes, total {}/{} err {}", handle, nwrote, nwritten, bytes.len(), GetLastError());


pcwalton · 2016-10-13T22:04:42Z

src/platform/windows/mod.rs

+            });
+
+            // if we had prematurely closed elements, just process them first
+            if selection_results.len() > 0 {


(BTW, there is .is_empty() if you'd like to use it for stuff like this; up to you)

pcwalton · 2016-10-13T22:04:52Z

src/platform/windows/mod.rs

+                    // function call itself failed or timed out.
+                    // Otherwise, the async IO operation failed, and
+                    // we want to hand io_err to notify_completion below.
+                    if ov_ptr == ptr::null_mut() {


Could be .is_null()

pcwalton · 2016-10-13T22:05:50Z

src/platform/windows/mod.rs

+
+impl From<WinError> for Error {
+    fn from(mpsc_error: WinError) -> Error {
+        //Error::new(ErrorKind::Other, format!("Win channel error ({} from {})", mpsc_error.0, mpsc_error.1))


Uncomment or remove

pcwalton · 2016-10-13T22:07:33Z

src/platform/windows/mod.rs

+        // if the buffer is full, add more space
+        let buf_len = self.read_buf.len();
+        let mut buf_cap = self.read_buf.capacity();
+        if buf_cap == buf_len {


nit: could do

if buf_cap == buf_len { return }

and save a level of indentation

vvuk · 2016-10-13T22:41:29Z

Awesome, thanks! Will look in detail later, but I like the nits - I'm still
learning rust, so seeing more idiomatic or cleaner ways to express
something is very much welcomed.

About the debug output - I was going to strip that, but I think I'll clean
it up and put it behind some kind of win32 IPC channel debug env.

On Oct 13, 2016 6:08 PM, "Patrick Walton" notifications@github.com wrote:

@pcwalton commented on this pull request.

I'm not an IOCP expert so I'll defer to @antrik
https://github.com/antrik for that, but I just had a few nits.

Very glad to see that this actually works in Win32 :)

In src/platform/windows/mod.rs
#108 (review):

};
+}

+macro_rules! dd { ($($rest:tt)) => { if *DD_ENABLED { println!($($rest)); } } }
+macro_rules! dd2 { ($($rest:tt)) => { if *DD_ENABLED || *DD2_ENABLED { println!($($rest)); } } }
+
+// We stick these in WinErrors to indicate various things
+// that aren't quite errors.
+const MAGIC_CODE_BASE: u32 = 0xdeadbee0u32;
+const MAGIC_NO_OUTSTANDING_DATA_CODE: u32 = MAGIC_CODE_BASE + 0;
+const MAGIC_CHANNEL_CLOSED_CODE: u32 = MAGIC_CODE_BASE + 1;
+
+// When we create the pipe, how big of a write buffer do we specify?
+// This is reserved in the nonpaged pool.
+//
+// This is 80k because a lot of the test assume that we can write a

uber-nit: "tests"

In src/platform/windows/mod.rs
#108 (review):

   dd2!("[$ {:?}:{:?}] notify_completion", self.iocp, self.handle);

   if err == winapi::ERROR_BROKEN_PIPE {

       assert!(!self.closed, "we shouldn't get an async BROKEN_PIPE after we already got one");

```
       self.closed = true;
```
```
       return Ok(());
```
```
   }
```

   let nbytes = self.ov.InternalHigh as u32;

```
   let offset = self.ov.Offset;
```
```
   assert!(offset == 0);
```
```
   // if the remote end closed...
```
```
   if err != winapi::ERROR_SUCCESS {
```

       panic!("[$ {:?}:{:?}] **\* notify_completion: need to handle error! {}", self.iocp, self.handle, err);

I guess add a FIXME here?

In src/platform/windows/mod.rs
#108 (review):

fn start_read(&mut self) -> Result<(),WinError> {

   if self.read_in_progress || self.closed {

```
       return Ok(());
```
```
   }
```

   dd2!("[$ {:?}:{:?}] start_read ov {:?}", self.iocp, self.handle, self.ov_ptr());

```
   let mut bytes_read: u32 = 0;
```

   // if the buffer is full, add more space

```
   let buf_len = self.read_buf.len();
```

   let mut buf_cap = self.read_buf.capacity();

```
   if buf_cap == buf_len {
```
```
       let more =
```

           if buf_cap == 0 { READ_BUFFER_SIZE }

       else if buf_cap < READ_BUFFER_MAX_GROWTH { buf_cap }

       else { READ_BUFFER_MAX_GROWTH };

Could be:

match buf_cap {
0 => READ_BUFFER_SIZE,
1...READ_BUFFER_MAX_GROWTH => buf_cap,
_ => READ_BUFFER_MAX_GROWTH,
}

In src/platform/windows/mod.rs
#108 (review):

```
   }
```

   let buf_header: &[u32] = unsafe { slice::from_raw_parts(self.read_buf.as_ptr() as *const u32, 2) };

   let data_bytes = buf_header[0] as usize;

   let oob_bytes = buf_header[1] as usize;

   let bytes_needed = HEADER_SIZE + data_bytes + oob_bytes;

   if self.read_buf.len() >= bytes_needed {

```
       Some((data_bytes, oob_bytes))
```
```
   } else {
```
```
       None
```
```
   }
```
}

// Err(false) -> something really failed
// Err(true) -> no message
// XXX This is dumb, we should return

nit: use FIXME

In src/platform/windows/mod.rs
#108 (review):

}
+}

+impl Clone for OsIpcSender {
fn clone(&self) -> OsIpcSender {
   OsIpcSender::from_handle(dup_handle(*self.handle).unwrap())
}
+}
+unsafe fn write_buf(handle: HANDLE, bytes: &[u8]) -> Result<(),WinError> {
let mut ntowrite: u32 = bytes.len() as u32;

if ntowrite == 0 {
   return Ok(());
}

let mut nwritten: u32 = 0;

//dd!("[c {:?}] writing: {:?}", handle, bytes);

Uncomment or remove

In src/platform/windows/mod.rs
#108 (review):

let mut nwritten: u32 = 0;
//dd!("[c {:?}] writing: {:?}", handle, bytes);
while nwritten < ntowrite {
```
   let mut nwrote: u32 = 0;
```
```
   if kernel32::WriteFile(handle,
```

                          bytes.as_ptr().offset(nwritten as isize) as LPVOID,

```
                          ntowrite,
```
```
                          &mut nwrote,
```

                          ptr::null_mut())

```
       == winapi::FALSE
```
```
   {
```

       return Err(WinError::last("WriteFile"));

```
   }
```
```
   nwritten += nwrote;
```
```
   ntowrite -= nwrote;
```

   //dd!("[c {:?}] ... wrote {} bytes, total {}/{} err {}", handle, nwrote, nwritten, bytes.len(), GetLastError());

Ditto

In src/platform/windows/mod.rs
#108 (review):

       // Make a quick first-run check for any closed receivers.

       // This will only happen if we have a receiver that

       // gets added to the Set after it was closed (the

       // router_drops_callbacks_on_cloned_sender_shutdown test

```
       // causes this.)
```
```
       receivers.retain(|ref r| {
```
```
           if r.is_closed() {
```

               selection_results.push(OsIpcSelectionResult::ChannelClosed(*r.handle as i64));

```
               false
```
```
           } else {
```
```
               true
```
```
           }
```
```
       });
```

       // if we had prematurely closed elements, just process them first

       if selection_results.len() > 0 {

(BTW, there is .is_empty() if you'd like to use it for stuff like this;

up to you)

In src/platform/windows/mod.rs
#108 (review):

           let mut completion_key: HANDLE = INVALID_HANDLE_VALUE;

           let mut ov_ptr: *mut winapi::OVERLAPPED = ptr::null_mut();

           // XXX use GetQueuedCompletionStatusEx to dequeue multiple CP at once!

           let ok = kernel32::GetQueuedCompletionStatus(*self.iocp,

                                                        &mut nbytes,

                                                        &mut completion_key as *mut _ as *mut u64,

                                                        &mut ov_ptr,

                                                        winapi::INFINITE);

           dd!("[# {:?}] GetQueuedCS -> ok:{} nbytes:{} key:{:?}", *self.iocp, ok, nbytes, completion_key);

           let mut io_err = winapi::ERROR_SUCCESS;

```
           if ok == winapi::FALSE {
```

               // If the OVERLAPPED result is NULL, then the

               // function call itself failed or timed out.

               // Otherwise, the async IO operation failed, and

               // we want to hand io_err to notify_completion below.

               if ov_ptr == ptr::null_mut() {

Could be .is_null()

In src/platform/windows/mod.rs
#108 (review):

}
fn ok_no_data() -> WinError {
   WinError::from_system(MAGIC_NO_OUTSTANDING_DATA_CODE, "no data pending")
}
+}
+impl From for DeserializeError {
fn from(mpsc_error: WinError) -> DeserializeError {
   DeserializeError::IoError(mpsc_error.into())
}
+}
+impl From for Error {
fn from(mpsc_error: WinError) -> Error {
   //Error::new(ErrorKind::Other, format!("Win channel error ({} from {})", mpsc_error.0, mpsc_error.1))

Uncomment or remove

In src/platform/windows/mod.rs
#108 (review):

```
   Ok(())
```
}

// kick off an asynchronous read
fn start_read(&mut self) -> Result<(),WinError> {

   if self.read_in_progress || self.closed {

```
       return Ok(());
```
```
   }
```

   dd2!("[$ {:?}:{:?}] start_read ov {:?}", self.iocp, self.handle, self.ov_ptr());

```
   let mut bytes_read: u32 = 0;
```

   // if the buffer is full, add more space

```
   let buf_len = self.read_buf.len();
```

   let mut buf_cap = self.read_buf.capacity();

```
   if buf_cap == buf_len {
```

nit: could do

if buf_cap == buf_len {
return
}

and save a level of indentation

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#108 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAL5lZJCCeIeRKOW_olV-F-hNZFWS9_-ks5qzqv0gaJpZM4KOU0X
.

vvuk · 2016-10-14T20:24:27Z

I put in a fixme comment for this, but -- do I need to protect against simultaneous OsIpcReceiverSet::add() and OsIpcReceiverSet::select() on different threads?

vvuk · 2016-10-20T16:01:56Z

Fixed the issue that was causing a problem with servo. With these fixes, multiprocess servo runs on Windows.

antrik · 2016-10-20T17:12:04Z

@vvuk when you find an issue that was not caught by the test suite, can you please try to create a new test case to cover it?...

Regarding your question about concurrent add() and select(), I don't think you need to do anything special there: Rust's type system should prevent such a situation. OsIpcReceiverSet is already !Sync AIUI, since it contains a Vec<>.

antrik · 2016-10-20T22:40:57Z

Regarding the benchmark results, these do not look too bad indeed. For comparison, here is what I get from a typical run on my (somewhat old) 32 bit GNU/Linux system:

test create_channel ... bench:       4,563 ns/iter (+/- 209)
test size_00_1      ... bench:       2,224 ns/iter (+/- 40)
test size_01_2      ... bench:       2,177 ns/iter (+/- 46)
test size_02_4      ... bench:       2,287 ns/iter (+/- 65)
test size_03_8      ... bench:       2,252 ns/iter (+/- 42)
test size_04_16     ... bench:       2,291 ns/iter (+/- 67)
test size_05_32     ... bench:       2,262 ns/iter (+/- 76)
test size_06_64     ... bench:       2,251 ns/iter (+/- 69)
test size_07_128    ... bench:       2,296 ns/iter (+/- 60)
test size_08_256    ... bench:       2,297 ns/iter (+/- 66)
test size_09_512    ... bench:       2,363 ns/iter (+/- 35)
test size_10_1k     ... bench:       2,490 ns/iter (+/- 71)
test size_11_2k     ... bench:       3,789 ns/iter (+/- 88)
test size_12_4k     ... bench:       5,412 ns/iter (+/- 71)
test size_13_8k     ... bench:       9,073 ns/iter (+/- 122)
test size_14_16k    ... bench:       8,852 ns/iter (+/- 92)
test size_15_32k    ... bench:      13,410 ns/iter (+/- 148)
test size_16_64k    ... bench:      21,967 ns/iter (+/- 3,849)
test size_17_128k   ... bench:      43,399 ns/iter (+/- 1,191)
test size_18_256k   ... bench:     174,217 ns/iter (+/- 26,285)
test size_19_512k   ... bench:     351,637 ns/iter (+/- 60,631)
test size_20_1m     ... bench:   1,451,128 ns/iter (+/- 282,506)
test size_21_2m     ... bench:   2,204,456 ns/iter (+/- 652,694)
test size_22_4m     ... bench:   5,975,498 ns/iter (+/- 1,106,189)
test size_23_8m     ... bench:  10,956,106 ns/iter (+/- 2,241,968)

I once did a few runs on a more modern system, which was about as fast as yours I guess. While I haven't published the results, IIRC they were mostly about 2 - 2.5x faster than the ones you get; except for medium sizes (around 128k - 512k or so), where the Linux implementation suffers less severely from fragmentation. (Even less on the modern system.) Channel creation also looks significantly faster on Linux.

From a quick glance, it looks like you do shuffle data around quite a bit in the reader etc. -- but given that you are using a packet-less transport, I suspect there isn't really too much room for optimisation there...

Does Windows have a simple way to see how much time is spent in system calls vs. userspace code (like the time command on Unix)? That should give you a rough idea whether a lot of time is spent in your code (so you might be able to squeeze out a bit more), or it's mostly the Windows IPC itself, which would mean there is probably nothing to be done about it...

antrik · 2016-10-20T22:43:28Z

@vvuk it seems you accidentally dropped the "Fix win32 nits" commit while pushing further changes?...

vvuk · 2016-10-21T03:48:17Z

Whoops. Quite. And re the tests -- in my hurry I convinced myself that there weren't already cross-process tests, but that's very dumb. They were if cfg'd out. They do use fork() unfortunately, which won't work on Windows, but I'll make them work tomorrow and clean up the branch a bit.

vvuk · 2016-10-21T17:38:19Z

Any ideas on how I can build a second binary that the tests will depend on? Or, alternatively, a way to give the test binary itself main() or something where I can control the args?

emilio · 2016-10-21T17:47:08Z

You can see an example here: https://github.com/servo/html5ever/blob/master/Cargo.toml

vvuk · 2016-10-24T18:29:50Z

That's better. Any final comments, and do you want to see this squashed?

antrik · 2016-10-24T20:16:36Z

@vvuk I'm still in the process of looking through the code (I'm trying to be thorough); but squashing is fine with me, as I haven't looked at the earlier version much, so I don't need to see what changed in the fix-up commits.

(The include change and the new test cases should remain separate commits, of course.)

emilio · 2016-10-25T18:16:07Z

@vvuk I think you can make a [[test]] target1.

Let me see if I find an example in the Servo repos.

On Fri, Oct 21, 2016 at 10:38:20AM -0700, Vladimir Vukicevic wrote:

Any ideas on how I can build a second binary that the tests will depend on? Or, alternatively, a way to give the test binary itself main() or something where I can control the args?

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#108 (comment)

bors-servo · 2016-10-25T23:50:07Z

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

antrik

So, uh... This is a shitload of questions, nitpicks, and assorted other remarks... Sorry for that: since I'm horribly perfectionist, I tend to comment an important things an minor details alike -- I hope you don't mind too much :-)

My major concern with this implementation is that sending descriptors along with data is not atomic, which I fear might cause serious problems... But I don't know whether a different approach is possible, since I actually have no clue about Windows programming. Maybe we need to summon a domain wizard? ;-)

Other than that, there are few other points that I'd consider quite important, though less fundamental -- while a lot of the remaining remarks are rather stylistic issues, which you might or might not agree with :-) (Though I'd love to hear what @pcwalton and others think of them...)

antrik · 2016-10-20T17:24:51Z

src/platform/mod.rs

+#[cfg(any(feature = "force-inprocess", target_os = "android"))]
+pub use self::inprocess::{OsIpcChannel, OsIpcOneShotServer, OsIpcReceiver,
+                          OsIpcReceiverSet, OsIpcSelectionResult, OsIpcSender,
+                          OsIpcSharedMemory, OsOpaqueIpcChannel, channel};


This makes it more similar again in some ways to what it was before b3386e3 . Can you please explain why you consider this an improvement?...

Without it, it's impossible to debug without doing a -Z debug-macros build -- all code goes to the include!() statement (see rust-lang/rust#36382).

Ugh, I see...

Why don't you just do pub use self::inprocess::* etc. though?

Also, this should be a separate PR I'd say?...

antrik · 2016-10-20T17:44:49Z

src/platform/windows/mod.rs

+}
+
+unsafe impl Send for WinHandle { }
+unsafe impl Sync for WinHandle { }


I take it the Windows API guarantees thread safety when using HANDLEs, i.e. running several system calls on the same HANDLE at the same time will never ever trigger undefined behaviour?

They are, though obviously what you do with them has to make sense in a multi-threading context.

antrik · 2016-10-20T18:00:56Z

src/platform/windows/mod.rs

+}
+
+unsafe impl Send for OsIpcReceiver { }
+unsafe impl Sync for OsIpcReceiver { }


The explicit Send declaration should not be necessary: as far as I can see, all the types used in this structure are automatically Send.

It's quite a different story for Sync though: since reader is a RefCell, which is not Sync, the containing structure isn't automatically Sync either -- and for good reasons, too! While I haven't tried understanding the implications 100%, it seems to me that attempting concurrent recv() calls would indeed race wildly with the current implementation... To actually make this thread-safe, you'd need to use some kind of mutex I guess.

In this specific case however you do not need to do this; but rather just drop the Sync claim: OsIpcReceiver should indeed not be Sync!

(There was a bug about this in the inprocess back-end -- see #107 )

HANDLE is neither Send nor Sync, which is still incredibly annoying. This needs to be marked Send. Should not be Sync, as you say.

Oh right, I forgot that you do have a raw HANDLE in there... Though as explained above, I think it would be better to get rid of it -- this is just one more reason :-)

antrik · 2016-10-20T18:02:36Z

src/platform/windows/mod.rs

+}
+
+unsafe impl Send for OsIpcSender { }
+unsafe impl Sync for OsIpcSender { }


No need to declare these explicitly -- it will automatically inherit these properties from the inner type.

(Actually, I even believe the ipc-channel senders should not be Sync at all, just as the mpsc senders aren't -- but that will have to happen separately as a major API change...)

I guess you have seen that I went ahead with this change in #115 ...

antrik · 2016-10-20T20:04:35Z

src/platform/windows/mod.rs

+        unsafe {
+            if self.handle.is_valid() {
+                kernel32::UnmapViewOfFile(self.ptr as LPVOID);
+            }


I don't know whether the descriptor is actually relevant for removing the mapping; but regardless, this condition seems weird: as far as I can see, there is no valid way to create or leave behind an OsIpcSharedMemory object without a valid descriptor? I'd say this should be an assert() at most...

antrik · 2016-10-28T04:00:46Z

src/platform/windows/mod.rs

+}
+
+impl From<WinError> for DeserializeError {
+    fn from(mpsc_error: WinError) -> DeserializeError {


I think you want win_error here, not mpsc_error :-)

antrik · 2016-10-28T04:10:26Z

src/platform/windows/mod.rs

+
+impl From<WinError> for Error {
+    fn from(mpsc_error: WinError) -> Error {
+        Error::new(ErrorKind::Other, format!("Win channel error ({}=0x{:x})", mpsc_error.0, mpsc_error.0))


Now that is very lazy :-P

It seems to me that you should special-case "channel closed" at the very least, like the mpsc back-end does.

Also, as far as I can see you should be able to use from_raw_os_error() for the real system error codes, just like the unix back-end?...

I didn't actually know how Error::from_raw_os_error and friends worked :) Fixed

antrik · 2016-10-28T05:05:46Z

src/platform/windows/mod.rs

+
+    // A pipe_id; if None, then this isn't a named
+    // pipe server (or is no longer one), and accept() will fail
+    pipe_id: Option<Uuid>,


See my remark about sender above: I don't think this really belongs here...

antrik · 2016-10-28T05:52:15Z

src/platform/windows/mod.rs

+}
+
+impl From<WinError> for Error {
+    fn from(mpsc_error: WinError) -> Error {


antrik · 2016-10-28T06:26:18Z

src/platform/windows/mod.rs

+    }
+
+    pub fn get_max_fragment_size() -> usize {
+        MAX_FRAGMENT_SIZE


Only having a lower bound here is a bit unfortunate, as some of the test cases rely on the precise amount for testing corner cases... (I was actually considering a scheme where this method would take the OOB item counts, in order to dynamically calculate the exact value -- before I realised that for the unix implementation this is actually orthogonal...)

Having said that, since this implementation works quite a bit differently, I'm not sure how relevant these particular corner cases actually are here...

vvuk · 2016-10-28T15:13:39Z

So, the non-atomic writes due to the header are definitely an issue, and I somehow managed to completely gloss over that. I'm going to write a test for that first and then probably need to memcpy the send data buffer unless it gets fragmented.

emilio · 2017-01-25T17:11:59Z

Seems like the only missing bit to land this is a rebase?

antrik · 2017-01-27T02:06:50Z

@emilio I'm very ashamed to say that I still haven't managed to finish going through the reworked code...

antrik

I finally managed to look though the newest code -- sorry it took so long again...

The good news is that I didn't find any new fundamental problems :-) I did find a few issues I consider pretty important (so at least the extra delay wasn't useless...) -- but these should all be pretty easy to fix without any major changes I believe...

I like the various improvements you did very much -- including those that do not seem to be in direct response to my comments :-)

It's a bit unfortunate that you squashed the test additions together with the new back-end implementation -- since the tests are actually independent of the new platform, I usually prefer them in a separate commit before the main one. (And only adding platform-specific conditionals in the main commit.) Not a serious problem though I'd say.

antrik · 2017-01-16T20:59:20Z

src/platform/windows/mod.rs

+    static ref DEBUG_TRACE_ENABLED: bool = { env::var_os("IPC_CHANNEL_WIN_DEBUG_TRACE").is_some() };
+}
+
+// some debug bump macros to better track what's going on in case of errors


Nitpick: still got that typo here... ("bump" should be "dump".)

antrik · 2017-01-16T21:35:10Z

src/platform/windows/mod.rs

+// passing, though that process would need to be global to any processes
+// amongst which you want to share channels or connect one-shot servers to.
+// There may be a system process that we could use for this purpose, but
+// I haven't foundone -- and in the system process case, we'd need to ensure


Nitpick: "foundone"

antrik · 2017-01-16T22:05:37Z

src/platform/windows/mod.rs

+}
+
+// duplicate a handle to the target process, closing the source handle
+fn move_handle_to_process(handle: &mut WinHandle, other_process: &WinHandle) -> Result<WinHandle,WinError> {


I wonder whether handle could be passed by value here? If so, it would make it more explicit that it's actually consumed...

(But even then, it's not a deal-breaker -- just a missed opportunity ;-) )

antrik · 2017-01-16T22:09:04Z

src/platform/windows/mod.rs

+        // FIXME this is not correct!  We need to compare the object
+        // the handles refer to.  On Windows 10, we have:
+        // unsafe { kernel32::CompareObjectHandles(self.h, other.h) == winapi::TRUE }
+        // But that 


Seems the comment got truncated?...

antrik · 2017-01-16T22:14:21Z

src/platform/windows/mod.rs

+}
+
+enum GetMessageResult {
+    NoMessage, 


Nitpick: there is trailing white space on this line.

(Git should give you a warning about this on commit I believe?... Or at least git diff displays such errors prominently if coloured output is enabled.)

antrik · 2017-01-30T05:32:23Z

src/platform/windows/mod.rs

+        // We limit the max size we can send here; we can fix this
+        // just by upping the header to be 2x u64 if we really want
+        // to.
+        assert!(data.len() < u32::max_value() as usize);


Shouldn't that be <=?

antrik · 2017-01-30T06:08:36Z

src/platform/windows/mod.rs

+
+        unsafe {
+            let in_band_data_len = if big_data_sender.is_none() { data.len() } else { 0 };
+            let full_in_band_len = MessageHeader::size() + in_band_data_len + oob_data.len();


These do not need to be inside the unsafe block, since the buffer is dereferenced safely (through slices) in the following code -- so a miscalculation here could cause a panic at worst.

(Not entirely sure about the following assert... What is the worst that could happen if the pipe buffer size is exceeded?)

antrik · 2017-01-30T06:25:32Z

src/platform/windows/mod.rs

+impl Drop for OsIpcSharedMemory {
+    fn drop(&mut self) {
+        unsafe {
+            kernel32::UnmapViewOfFile(self.ptr as LPVOID);


I think we need to check for errors here?

(Conditionally, to avoid double panics -- see Unix implementation for reference...)

antrik · 2017-01-30T06:34:03Z

src/platform/windows/mod.rs

+    fn new(length: usize) -> Result<OsIpcSharedMemory,WinError> {
+        unsafe {
+            assert!(length < u32::max_value() as usize);
+            let (lhigh, llow) = (0 as u32, (length & 0xffffffffusize) as u32);


I fear there must have been some kind of misunderstanding here? We certainly do not want to limit the size of mappings to 32 bits -- I just said that there is no need to special-case for 32 bit systems, since the bit shift for lhigh naturally produces the correct effect with #[allow(exceeding_bitshifts)]...

I've read up on the exceeding bitshifts situation (ugh); and I see now that we do indeed need some kind of explicit handling here -- sorry for this.

Still, I think the original approach with #[allow(exceeding_bitshifts)] was pretty fragile and confusing. I'd vote for more explicit conditional compilation, like this:

#[cfg(target_pointer_width = "64")] let (lhigh, llow) = ((length >> 32) as u32, (length & 0xffffffff) as u32); #[cfg(target_pointer_width = "32")] let (lhigh, llow) = (0, length as u32);

As an additional advantage, if we ever encounter a different pointer size, this will fail at compile time, rather than producing something unexpected. (Might be a hypothetical case -- but better safe than sorry...)

Yet another option is using checked_shr():

let (lhigh, llow) = (length.checked_shr(32).unwrap_or(0) as u32, (length & 0xffffffff) as u32);

This seems the simplest and cleanest option by far. It might result in a branch being performed at run time, unless the compiler is smart enough to elide it -- but the overhead should be insignificant (especially since it's a predictable branch); and I'd say the simplification justifies it...

antrik · 2017-01-30T07:02:09Z

src/platform/windows/mod.rs

+        match *self {
+            WinError::ChannelClosed => true,
+            _ => false,
+        }


If you derive PartailEq on WinError, you could just use an ordinary comparison here, like in the inprocess back-end.

This implementation uses named pipes on Windows, with auto-generated uuid names. It takes advantage of DuplicateHandle to clone handles to target processes when sending handles cross-process. Shared memory is implemented using anonymous file mappings. select() and friends are implemented using IO Completion Ports.

…ot correct

… due to Mach error

reset · 2017-05-27T18:03:33Z

Hey all, just curious what the state of this PR is? It looks like @antrik's feedback was addressed and the PR was rebased. I could carry this PR across the finish line if there's something else needed - this is something we need in the habitat project and I'd love to get it out of the way.

Appreciate all the hard work that's gone into this PR both in the code and the review!

antrik · 2017-05-27T21:10:33Z

@reset great to see other users of ipc-channel beside Servo itself :-)

I hadn't seen the latest update. However, unless I'm missing something, it doesn't seem to address my latest review at all?...

Admittedly, many of these remarks are nitpicks, that I only brought up for completeness, and probably wouldn't have mentioned if there was nothing else to warrant commenting. Another batch however are code robustness concerns, which are (hopefully) not actual bugs right now -- but bearing a risk of producing soundness issues when related code is modified in the future... Personally, I'm not very comfortable committing such things. Last but not least, I believe there are a few genuine (though admittedly minor) defects.

None of these are real show-stoppers -- if someone decides to merge as is (after squashing), I wouldn't be outraged... Only somewhat disappointed ;-) As these are all pretty straightforward to fix I believe, it would be a pity IMHO not to do so...

Frankly, since they seem so straightforward to me, I'm seriously considering just making the changes myself... My only concern is that if I don't get it right on the first try, it would be pretty much impossible for me to debug it, without actually having a Windows system at hand.

reset · 2017-05-27T21:53:13Z

You're right, there was so much content here that I assumed the last push addressed your remarks. I'm very ok with doing that work as well if you're busy! Super grateful that you all did all this work already. We are about to make a largish change to our process supervisor and need the IPC channel for all three platforms. Thank you so much for all this work! Let me know if you'd like me to do the cleanup or if you'll handle it. Either way is very cool with me!

…

On Sat, May 27, 2017 at 2:10 PM Olaf Buddenhagen ***@***.***> wrote: @reset <https://github.com/reset> great to see other users of ipc-channel beside Servo itself :-) I hadn't seen the latest update. However, unless I'm missing something, it doesn't seem to address my latest review at all?... Admittedly, many of these remarks are nitpicks, that I only brought up for completeness, and probably wouldn't have mentioned if there was nothing else to warrant commenting. Another batch however are code robustness concerns, which are (hopefully) not actual bugs right now -- but bearing a risk of producing soundness issues when related code is modified in the future... Personally, I'm not very comfortable committing such things. Last but not least, I believe there are a few genuine (though admittedly minor) defects. None of these are real show-stoppers -- if someone decides to merge as is (after squashing), I wouldn't be outraged... Only somewhat disappointed ;-) As these are all pretty straightforward to fix I believe, it would be a pity IMHO not to do so... Frankly, since they seem so straightforward to me, I'm seriously considering just making the changes myself... My only concern is that if I don't get it right on the first try, it would be pretty much impossible for me to debug it, without actually having a Windows system at hand. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#108 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADTFA0rg7NKxGk7M69CkFdZ4EjTY5dtks5r-JFKgaJpZM4KOU0X> .

vvuk · 2017-05-27T22:41:07Z

Jamie, I'm happy to help as well with advice etc - my feeling here was that it was in good enough shape to merge, perhaps with a note saying that there are some outstanding issues and maybe a non-default feature and then fix up issues as they come up over time instead of blocking the bulk of the work. I just can't commit to doing the rest of the work at this point. I'll double check my laptop to make sure I don't have any outstanding work here and make sure my win32 branch is up to date.

antrik · 2017-05-28T00:51:41Z

@reset thanks for the offer -- though TBH, most of the time I'm not really all that busy, but rather distracted... Since I have already thought about this stuff a lot, and know exactly what I'm after, I think it would probably be more efficient if I give it a try myself first, and only ask for assistance if I need help debugging?...

If however I fail to follow up within a week or so, feel free to push for this being merged as-is (after squashing), and/or to pick up from here yourself.

(BTW, does anyone happen to know whether there is a simple way to "cargo check" for a different target platform?...)

reset · 2017-05-28T08:33:55Z

That sounds great to me @antrik. I'll follow up in about a week or so!

There's no cargo check for another platform, but on the Habitat project we've purchased licenses for Windows and VMWare fusion for all of our developers seeing that we need to support multiple platforms. We also setup CI with appveyor for testing on Windows which has worked well for us. It's easy to setup and well worth the time once you delve into adding features for Windows. I'm not a Windows person myself outside of playing and making games on the platform so this was all pretty new to me. If you all need any help or advice just reach out!

antrik · 2017-06-02T07:46:48Z

I created a new WIP PR in #166 . It doesn't actually contain the proposed code changes yet; however, it refactors the original commits -- which should make it a better starting point for further work, and/or final merging.

(Also, leaving behind the giant existing discussion thread here, should reduce incidents of angry unicorn...)

bors-servo · 2017-08-22T00:17:07Z

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

reset · 2017-09-27T19:16:23Z

@antrik the Habitat project has been using this branch to get Windows support since late May/early June without a hitch. We'd love to be able to receive updates from master but we're on this branch until it's merged.

I'd love to help see this over the finish line or at the very least push up some changes which fix the conflicting files so we can branch this branch and maintain a separate master branch until this is merged in. @antrik would you be alright with that or do you want me to hold off?

antrik · 2017-09-27T19:59:43Z

@reset I've been working on this on-and-off over the past months -- making improvements, as well as rebasing on master regularly -- over in #166 ; in fact I made some pretty good progress just recently :-)

The top commit in this branch is often broken though, since I'm trying new stuff I can't test locally... However, it would be easy for me to maintain a separate windows-stable branch with only the tested stuff. Would that help you for the time being?

BTW, I'd prefer all new discussion to happen in #166 (not least because my browser seriously struggles with this overly long discussion thread...) -- in fact, it would probably be best to close this obsolete PR now in favour of the active one...

jdm · 2017-09-27T22:00:12Z

Let's go ahead and close this, in that case.

vvuk force-pushed the win32 branch 2 times, most recently from af63d10 to 686f358 Compare October 5, 2016 02:49

antrik reviewed Oct 5, 2016

View reviewed changes

vvuk mentioned this pull request Oct 6, 2016

Support multiprocess constellation on Windows servo/servo#13609

Closed

4 tasks

pcwalton reviewed Oct 13, 2016

View reviewed changes

vvuk force-pushed the win32 branch from d40aa23 to fd5ad4c Compare October 14, 2016 20:21

vvuk force-pushed the win32 branch from fd5ad4c to d2f9e3b Compare October 20, 2016 16:01

vvuk force-pushed the win32 branch from cdeacfd to 1476fbc Compare October 27, 2016 02:01

antrik reviewed Oct 28, 2016

View reviewed changes

antrik reviewed Jan 30, 2017

View reviewed changes

vvuk added 6 commits April 11, 2017 14:22

Add inception channel-in-channel multi-process transfer test

bd0276c

Windows: Track the intended target process id for handles; panic if n…

5d89341

…ot correct

Verify that two_step/channelception test panics on Windows

f13c941

Windows: fix up review comments

3ef1c3f

Flag cross_process_two_step_transfer_spawn as ignored on OSX as well,…

d0488a3

… due to Mach error

vvuk force-pushed the win32 branch 3 times, most recently from c36049a to 1b014ce Compare April 11, 2017 21:56

Update for recent changes

1b014ce

This was referenced Jun 1, 2017

Add Stream and Sink future traits to IpcSender and Receiver #165

Open

[WIP] Add native Windows back-end #166

Closed

highfive added the S-needs-rebase label Aug 22, 2017

jdm closed this Sep 27, 2017

doitian mentioned this pull request Jan 15, 2018

Update README.md since Windows is supported #186

Closed

grovesNL mentioned this pull request May 1, 2019

remote: handle IPC on Windows gfx-rs/wgpu#146

Closed

Implement ipc-channel on Windows #108

Implement ipc-channel on Windows #108

Conversation

vvuk commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antrik commented Oct 5, 2016

vvuk commented Oct 6, 2016

vvuk commented Oct 12, 2016

antrik commented Oct 13, 2016

vvuk commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcwalton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvuk commented Oct 13, 2016

Very glad to see that this actually works in Win32 :)

uber-nit: "tests"

I guess add a FIXME here?

nit: use FIXME

Uncomment or remove

Ditto

up to you)

Could be .is_null()

Uncomment or remove

vvuk commented Oct 14, 2016

vvuk commented Oct 20, 2016

antrik commented Oct 20, 2016

antrik commented Oct 20, 2016

antrik commented Oct 20, 2016

vvuk commented Oct 21, 2016

vvuk commented Oct 21, 2016

emilio commented Oct 21, 2016

vvuk commented Oct 24, 2016

antrik commented Oct 24, 2016

emilio commented Oct 25, 2016

bors-servo commented Oct 25, 2016

antrik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvuk Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvuk commented Oct 28, 2016

emilio commented Jan 25, 2017

antrik commented Jan 27, 2017

antrik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvuk Oct 28, 2016 •

edited

Loading

reset commented May 28, 2017 •

edited

Loading