Windows (rebased and updated) #272

tdgne · 2021-03-02T01:13:19Z

This is a rebase of #233 with some updates, namely

fixed unresolved dependencies
impls of error conversions
remaining work explained in Windows #233 (comment)

Existing tests passed on my Windows 10 PC with toolchain nightly-x86_64-pc-windows-msvc running cargo test --features="windows-shared-memory-equality,unstable", cargo test --features="windows-shared-memory-equality" and cargo test --features="force-inprocess,windows-shared-memory-equality" so far.

Happened to need this, hope it helps...

…fer() Since we assert the number of elements in the vector to be exactly one right before this, we can just index the element directly.

Since `fork()` isn't available on Windows, we need to do the tests using `spawn()` instead. The `spawn()` variants work both on Windows and on Unix platforms (including MacOS), making the `fork()` variants pretty redundant. However, it doesn't really hurt to keep them around, just in case. (It might help narrowing down any problems coming up, for example.)

In preparation for introducing spawn tests using more than one channel, we need a way to extract specific channel name args by distinctive labels. All existing `*_spawn()` tests are adapted to include the label in the passed command line argument; and all `*_server()` helpers are updated with the new argument in the `get_channel_name_arg()` calls.

In preparation for adding `_spawn()` test variants for the tests in `src/test.rs` as well, we need to move the helper there, so it can be used in both `src/test.rs` and `src/platform/test.rs` -- just like the `fork()` helpers.

Just like the other cross-process tests, this one needs a variant using `spawn()` instead of `fork()`, so it can work on platforms that don't provide `fork()`.

Change the way parameter unwrapping / conditional execution is handled, so the active code in the `*_server()` functions is more in line with the corresponding `*_fork()` variants. This should faciliate keeping the fork/spawn variants in sync.

Integrate the server functionality right into the main `_spawn()` test functions: running either the server or the client portions as necessary. This further aligns the structure of these tests with the corresponding `_fork()` variants. It also avoids the need for the `#[ignore]`/`--ignored` hack, since all the test functions now always run -- either as normal tests (client parts); or as pseudo-tests, when self-spawned in server mode.

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.

Move the check for finding equality of cloned/received SHM objects -- known to fail on Windows -- into a separate test case, rather than using a conditional in the main SHM test case. This makes it more explicit that this is a know limitation, and distinct from other SHM functionality. Also, more explicitly document this limitation in the relevant part of the Windows back-end implementation itself, rather than elaborating on it in the test case.

Turn comments describing the purpose of data types, functions etc. into proper doc comments. Also restructure them as needed to fit the expected form for doc comments; and in some cases, clarify/enhance the contents too.

Like in the other back-ends, remove (`mem::replae()`) the original handle while constructing the wrappers, to make sure we do not keep around a copy that might interfere if reused accidentally.

Use `&self` rather than `&mut self` in some private methods that do not actually modify the main object. (Mostly because they rely on inner mutability.) This also removes the need for `mut` in a few places in the callers.

This method could yield uninitialised data when invoked incorrectly.

Move some more calculations inside the unsafe block, since the other unsafe code relies upon them to be correct in order for soundness to be ensured.

Just use a tuple in an `Option<>` instead. The named type didn't really add anything here -- it only made the code harder to follow. (The original introduction of this custom type resulted from a communication failure...)

Extend the buffer's exposed length to cover its entire allocated capacity before using it in receive calls, so we can use safe slice operations rather than manual pointer arithmetic for determining addresses and lengths to be passed to the system calls. To keep the effects localised, we reset the length to the actually filled part again after the system calls, rather than keeping it at the full allocated size permanently. I'm not entirely sure yet whether to consider that more or less defensive than the other option -- but at least I'm confident that it's more robust than the original approach.

No need to generate spare heat while I'm working only on the windows back-end...

Make sure any outstanding async I/O operation is cancelled before freeing the memory of the `ov` structure and receive buffer. This is an important safety fix: leaving the operation pending after the memory is freed might result in the kernel later writing to memory locations that are no longer valid.

When moving the associated handle to another process, immediately consume the `MessageReader` containing it, so we do not leave the reader hanging around with an invalid handle. While this doesn't really change much in practice -- the reader was dropped along with the receiver shortly after anyway -- it's cleaner semantically, and should be a tick more robust.

More explicitly reflect the fact that this function consumes (moves) the original handle.

The temporary duplicate was only necessary as a workaround, to prevent the new `OsIpcReceiver` copy from ending up with an invalid handle, when the original receiver gets dropped. (Specifically, the original receiver gets dropped after serialisation, before the actual transfer happens using the copy...) However, now we just can use inner mutability to actually unset the handle value in the original `OsIpcReceiver` struct (since the handle now lives inside a `RefCell` as part of `MessageReader`) -- so there is no longer a danger of the handle being dropped prematurely along with the original receiver.

`OsIpcSender` is automatically `Sync`, since all its constituents are. We aren't doing anything on top of that to ensure it can indeed be shared -- so declaring `Sync` explicitly is wrong, and could obscure problems if the inherent `Sync` properties ever change for some reason.

tdgne · 2021-03-02T06:25:40Z

Oops! Looks like a bunch of tests won't compile when windows-shared-memory-equality feature is turned off.
I think I'll add some attribute like #[cfg(any(not(target_os = "windows"), all(target_os = "windows", feature = "windows-shared-memory-equality")))] to those later.

…lity` is absent

jdm · 2021-03-02T13:46:38Z

Hmm, 32 bit mingw has one test failure:

---- test::router_big_data stdout ----
thread 'test::router_big_data' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', src\test.rs:426:58
stack backtrace:
   0: core::fmt::write
             at src\libcore\fmt/mod.rs:1076
   1: std::io::Write::write_fmt
             at /rustc/f781babf87dea29c44f93842b7ac9eb809549d29\src\libstd\io/mod.rs:1537
   2: std::io::impls::<impl std::io::Write for alloc::boxed::Box<W>>::write_fmt
             at src\libstd\io/impls.rs:176
   3: std::sys_common::backtrace::_print
             at src\libstd\sys_common/backtrace.rs:62
   4: std::sys_common::backtrace::print
             at src\libstd\sys_common/backtrace.rs:49
   5: std::panicking::default_hook::{{closure}}
             at src\libstd/panicking.rs:198
   6: std::panicking::default_hook
             at src\libstd/panicking.rs:214
   7: std::panicking::rust_panic_with_hook
             at src\libstd/panicking.rs:520
   8: rust_begin_unwind
             at src\libstd/panicking.rs:431
   9: core::panicking::panic_fmt
             at src\libcore/panicking.rs:85
  10: core::option::expect_none_failed
             at src\libcore/option.rs:1269
  11: core::result::Result<T,E>::unwrap
             at /rustc/f781babf87dea29c44f93842b7ac9eb809549d29\src\libcore/result.rs:1005
  12: ipc_channel::test::router_big_data
             at src/test.rs:426
  13: ipc_channel::test::router_big_data::{{closure}}
             at src/test.rs:411
  14: core::ops::function::FnOnce::call_once
             at /rustc/f781babf87dea29c44f93842b7ac9eb809549d29\src\libcore\ops/function.rs:233
  15: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once
             at /rustc/f781babf87dea29c44f93842b7ac9eb809549d29\src\liballoc/boxed.rs:1081
  16: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
             at /rustc/f781babf87dea29c44f93842b7ac9eb809549d29\src\libstd/panic.rs:318
  17: std::panicking::try::do_call
             at /rustc/f781babf87dea29c44f93842b7ac9eb809549d29\src\libstd/panicking.rs:342
  18: std::panicking::try
             at /rustc/f781babf87dea29c44f93842b7ac9eb809549d29\src\libstd/panicking.rs:319
  19: std::panic::catch_unwind
             at /rustc/f781babf87dea29c44f93842b7ac9eb809549d29\src\libstd/panic.rs:394
  20: test::run_test_in_process
             at src\libtest/lib.rs:541
  21: test::run_test::run_test_inner::{{closure}}
             at src\libtest/lib.rs:450
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
failures:
    test::router_big_data

tdgne · 2021-03-02T17:47:53Z

The test::router_big_data failure reproduced locally (also in msvc builds...) but its pretty strange.

It reproduced when running cargo test --features="unstable,force-inprocess" but not cargo test --features="unstable" or even cargo test test::router_big_data --features="unstable,force-inprocess" 🤔

Two other tests involving the router turned out to fail occasionally too.

test test::router_multiplexing ... FAILED
test test::router_multithreaded_multiplexing ... FAILED

Seemingly related to #106

Trying to investigate this...

…bal ROUTER

tdgne · 2021-03-09T14:47:05Z

Ok, I found out why those tests occasionally failed.

One of the tests involving the global ROUTER shuts it down.
Because tests could be run in parallel, sometimes the ROUTER would be shut down before the other tests using ROUTER complete.

I've fixed that by making only one test use the global ROUTER and others use a local RouterProxy, wonder if that's all right.

jdm · 2021-03-10T02:31:43Z

Ok, I found out why those tests occasionally failed.

One of the tests involving the global ROUTER shuts it down.
Because tests could be run in parallel, sometimes the ROUTER would be shut down before the other tests using ROUTER complete.

I've fixed that by making only one test use the global ROUTER and others use a local RouterProxy, wonder if that's all right.

Thanks for investigating this and figuring it out! The explanation makes sense, and the fix sounds reasonable to me.

jdm · 2021-03-10T02:32:42Z

@bors-servo r+

bors-servo · 2021-03-10T02:32:43Z

📌 Commit 2c88763 has been approved by jdm

bors-servo · 2021-03-10T02:32:45Z

⌛ Testing commit 2c88763 with merge 51ac27f...

bors-servo · 2021-03-10T02:59:03Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: jdm
Pushing 51ac27f to master...

sunjay · 2021-03-12T21:50:01Z

Nice! Thanks for taking the time to get this through! 🥳🎉

vvuk · 2021-03-12T21:50:23Z

@tdgne thank you for doing this!

ofek · 2021-03-13T01:02:03Z

Thanks!!!

ofek · 2021-03-13T01:21:36Z

Could a bit be added about the Windows approach?

ipc-channel/src/lib.rs

Lines 12 to 15 in 51ac27f

    
           //! An implementation of the Rust channel API over process boundaries. Under the 
        
           //! hood, this API uses Mach ports on Mac and file descriptor passing over Unix 
        
           //! sockets on Linux. The serde library is used to serialize values for transport 
        
           //! over the wire.

vvuk and others added 30 commits February 27, 2021 19:04

tests: cleanup: Avoid .pop().unwrap() in cross_process_sender_trans…

cfc4336

…fer() Since we assert the number of elements in the vector to be exactly one right before this, we can just index the element directly.

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

2079935

tests: Move get_channel_name_arg() helper to src/test.rs

a78db14

In preparation for adding `_spawn()` test variants for the tests in `src/test.rs` as well, we need to move the helper there, so it can be used in both `src/test.rs` and `src/platform/test.rs` -- just like the `fork()` helpers.

tests: Introduce cross_process_embedded_senders_spawn()

a37e29e

Just like the other cross-process tests, this one needs a variant using `spawn()` instead of `fork()`, so it can work on platforms that don't provide `fork()`.

tests: cleanup: Introduce a common spawn_server() helper function

f8e539d

appveyor.yml: Restore testing inprocess back-end, not only native one

3cdb508

windows: fix warning: reader doesn't need to be mutable

480944b

windows: cleanup: Use more standard indentation for macro

097f001

windows: Move out original handle in to_receiver() and to_sender()

340756e

Like in the other back-ends, remove (`mem::replae()`) the original handle while constructing the wrappers, to make sure we do not keep around a copy that might interfere if reused accidentally.

windows: Drop unnecessary &mut self in a few places

a6d3ce5

Use `&self` rather than `&mut self` in some private methods that do not actually modify the main object. (Mostly because they rely on inner mutability.) This also removes the need for `mut` in a few places in the callers.

windows: cleanup: Switch methods for a more logical order

a29b090

windows: Mark notify_completion() as unsafe

5f9209a

This method could yield uninitialised data when invoked incorrectly.

windows: Widen too narrow unsafe block

49fc93c

Move some more calculations inside the unsafe block, since the other unsafe code relies upon them to be correct in order for soundness to be ensured.

windows: Drop custom GetMessageResult type

6c913c1

Just use a tuple in an `Option<>` instead. The named type didn't really add anything here -- it only made the code harder to follow. (The original introduction of this custom type resulted from a communication failure...)

[RemoveMe] Temporarily disable most CI targets

3facdd3

No need to generate spare heat while I'm working only on the windows back-end...

windows: Add explanation why the ov structure needs to be boxed

91e72d2

windows: Take ownership of handle in move_handle_to_process()

073423d

More explicitly reflect the fact that this function consumes (moves) the original handle.

windows: Assert valid state before consuming receiver

e2de7f6

tdgne added 4 commits March 2, 2021 14:47

Add test configurations

fb240b9

Clean up conditions

6dfac4a

Bump version

335a95b

Remove an item from missing features

1049140

Disable some tests for Windows if feature `windows-shared-memory-equa…

2b2e72e

…lity` is absent

Fix test cases involving router so that only one of them uses the glo…

2c88763

…bal ROUTER

tdgne requested a review from jdm March 9, 2021 14:47

highfive assigned jdm and unassigned emilio Mar 10, 2021

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 10, 2021

bors-servo merged commit 51ac27f into servo:master Mar 10, 2021

highfive removed the S-awaiting-merge label Mar 10, 2021

This was referenced Mar 12, 2021

Windows #233

Closed

[Do Not Merge] Add native Windows back-end (stable branch) #169

Closed

[WIP] Add native Windows back-end #166

Closed

demurgos mentioned this pull request Apr 23, 2021

Windows support #36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows (rebased and updated) #272

Windows (rebased and updated) #272

tdgne commented Mar 2, 2021 •

edited

Loading

tdgne commented Mar 2, 2021

jdm commented Mar 2, 2021

tdgne commented Mar 2, 2021 •

edited

Loading

tdgne commented Mar 9, 2021

jdm commented Mar 10, 2021

jdm commented Mar 10, 2021

bors-servo commented Mar 10, 2021

bors-servo commented Mar 10, 2021

bors-servo commented Mar 10, 2021

sunjay commented Mar 12, 2021

vvuk commented Mar 12, 2021

ofek commented Mar 13, 2021

ofek commented Mar 13, 2021

Windows (rebased and updated) #272

Windows (rebased and updated) #272

Conversation

tdgne commented Mar 2, 2021 • edited Loading

tdgne commented Mar 2, 2021

jdm commented Mar 2, 2021

tdgne commented Mar 2, 2021 • edited Loading

tdgne commented Mar 9, 2021

jdm commented Mar 10, 2021

jdm commented Mar 10, 2021

bors-servo commented Mar 10, 2021

bors-servo commented Mar 10, 2021

bors-servo commented Mar 10, 2021

sunjay commented Mar 12, 2021

vvuk commented Mar 12, 2021

ofek commented Mar 13, 2021

ofek commented Mar 13, 2021

tdgne commented Mar 2, 2021 •

edited

Loading

tdgne commented Mar 2, 2021 •

edited

Loading