Windows #233

angelortiz1007 · 2019-06-24T21:35:19Z

Preliminary IPC-Channel modifications based on rust 2018. It is also a rebase of #166.

…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.

This change vendors `wgpu` library in-tree and hooks up the initialization bits. It implements adapter and device initialization and adds a simple test. Current status: - [x] Architecture - [x] figure out the IPC story - [ ] move wgpu crates into a dedicated folder (let's follow up with this) - [x] Review - [x] WebIDL changes by DOM peers - [x] Linux - [x] avoid depending on spirv_cross - gfx-rs/wgpu#371 - [x] macOS - [x] due to cross-compiling shaders - gfx-rs/gfx#3047 - [x] need the dependency update - [x] stop using gcc - SSheldon/rust-objc-exception#5 - [x] unexpected SSL header collision - https://phabricator.services.mozilla.com/D51148 - [x] undefined Metal symbols - [x] missing webrtc headers for IPDL magic - https://phabricator.services.mozilla.com/D51558 - [x] Windows - [x] due to "ipc-channel" not supporting Windows yet - servo/ipc-channel#233 - [x] due to some exceptional stuff - grovesNL/spirv_cross#121 - [x] undefined symbol: `D3D12CreateDevice` - [x] d3d12.dll is not found, dxgi1_4 doesn't present - [x] d3d11.dll and dxgi.dll need to be explicitly loaded on win32 mingw - gfx-rs/gfx#3076 - [x] libbacktrace fails to link on win32 mingw - [x] cc mislinking C++ standard library - rust-lang/cc-rs#455 - [x] Android - [x] spirv-cross fails to build - KhronosGroup/SPIRV-Cross#1193 Update-1: We decided to go with IPDL mechanism instead of Rust based ipc-channel (or any alternatives), which unblocks Windows build. Update-2: It appears that WebGPUThreading isn't needed any more as the child thread (and its event loop) is now managed by IPDL infrastructure. This PR removes it 🎉 . Update-3: InstanceProvider is also removed. Update-4: All set, the try is green, waiting for dependent changes to go in. Differential Revision: https://phabricator.services.mozilla.com/D49458

This change vendors `wgpu` library in-tree and hooks up the initialization bits. It implements adapter and device initialization and adds a simple test. Current status: - [x] Architecture - [x] figure out the IPC story - [ ] move wgpu crates into a dedicated folder (let's follow up with this) - [x] Review - [x] WebIDL changes by DOM peers - [x] Linux - [x] avoid depending on spirv_cross - gfx-rs/wgpu#371 - [x] macOS - [x] due to cross-compiling shaders - gfx-rs/gfx#3047 - [x] need the dependency update - [x] stop using gcc - SSheldon/rust-objc-exception#5 - [x] unexpected SSL header collision - https://phabricator.services.mozilla.com/D51148 - [x] undefined Metal symbols - [x] missing webrtc headers for IPDL magic - https://phabricator.services.mozilla.com/D51558 - [x] Windows - [x] due to "ipc-channel" not supporting Windows yet - servo/ipc-channel#233 - [x] due to some exceptional stuff - grovesNL/spirv_cross#121 - [x] undefined symbol: `D3D12CreateDevice` - [x] d3d12.dll is not found, dxgi1_4 doesn't present - [x] d3d11.dll and dxgi.dll need to be explicitly loaded on win32 mingw - gfx-rs/gfx#3076 - [x] libbacktrace fails to link on win32 mingw - [x] cc mislinking C++ standard library - rust-lang/cc-rs#455 - [x] Android - [x] spirv-cross fails to build - KhronosGroup/SPIRV-Cross#1193 Update-1: We decided to go with IPDL mechanism instead of Rust based ipc-channel (or any alternatives), which unblocks Windows build. Update-2: It appears that WebGPUThreading isn't needed any more as the child thread (and its event loop) is now managed by IPDL infrastructure. This PR removes it 🎉 . Update-3: InstanceProvider is also removed. Update-4: All set, the try is green, waiting for dependent changes to go in. Differential Revision: https://phabricator.services.mozilla.com/D49458 UltraBlame original commit: cbec8a55a1bbad484ea0628b527de6f0cc7dc5f7

bors-servo · 2019-12-20T20:46:56Z

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

ofek · 2019-12-24T21:45:19Z

This is exciting!!!

sunjay · 2020-05-07T06:11:40Z

Thanks for everyone's work on this! Any update on this? ipc-channel seems to be one of the best crates for doing IPC in Rust right now. Would be great to get Windows support so it can be used cross-platform. :)

antrik · 2020-05-08T16:15:31Z

I guess I should really get back on this one of these days... My previous work was almost ready to merge (just missing a proper commit message for my latest change, and perhaps some additional test cases) -- but after I have been away for a long time, and new issues came up that I will need to dig into, the thought of getting back to it somewhat scares me :-(

The other thing is that with all the changes I have done to @vvuk 's original work, it probably needs another full review... Ideally, @vvuk himself, or someone else understanding this stuff, could look at all the commits aside from the one incorporating @vvuk 's original patch -- though at this point, maybe it would be more reasonable to just review the end result, with no regard for the history?...

jdm · 2020-05-08T16:18:16Z

#233 (comment) remains a summary of the work that is required here, to the best of my knowledge.

BatmanAoD · 2020-07-08T23:09:53Z

@antrik is your work-so-far in a public branch somewhere? Which of the issues in @jdm's comment does it address (fully or partially)?

vvuk · 2020-07-08T23:25:05Z

I think you just want to look at the end result. The history doesn't really matter at this point. But I'm probably not a good reviewer; I am very far from this at this point, and (sadly) not doing much Rust lately.

Also, the origin of this work is now 4 years old. Was it really worth not landing the bulk of the work in an imperfect state, and then letting people make incremental improvements & fixes? Hopefully this doesn't go another 4 years.

Boscop · 2021-01-10T16:02:03Z

Any update on this? :)

Which fork / branch should I use if I need windows support today?

jdm · 2021-01-10T18:22:33Z

No update. #233 (comment) describes the work that remains for this branch.

Boscop · 2021-01-10T18:26:53Z

@jdm But aren't people using some branch of this already in production (so it's working on windows)? E.g. habitat.

jdm · 2021-01-10T23:35:35Z

I have no idea if anybody is using this branch. Servo is not.

vvuk · 2021-01-11T01:58:21Z

Does that mean that Servo is (still) not multiprocess on Windows? Or is it doing an alternative approach instead of ipc-channel for communication?

jdm · 2021-01-11T06:15:41Z

Servo is still not multiprocess on Windows.

vvuk · 2021-01-11T06:20:30Z

That seems like a very poor outcome, because I had multiprocess servo working on Windows with a previous version of this ipc-channel work in October 2016, over 4 years ago. I know Windows was never important for Servo (and still isn't), but I think this is doing a disservice to Windows users and developers. Then again, the work described in comment #233 seems reasonable, but I'm really hesitant to give it a shot because of how this went last time. It was a pretty demoralizing experience.

Boscop · 2021-01-11T07:11:43Z

@vvuk Could you please try to make it work?
I would really appreciate to have this crate working on windows. (I would offer my help but this is outside of my expertise.)
It would be a shame if all this effort didn't get merged.
(I agree, that the last working state should have been merged, and further improvements could have been done in separate PRs.)

jdm · 2021-01-12T01:09:56Z

If someone finishes the work I'll merge it. I no longer have access to a Windows development environment.

ofek · 2021-01-12T03:05:03Z

I would still love to use this on Windows!

Windows (rebased and updated) This is a rebase of #233 with some updates, namely * fixed unresolved dependencies * impls of error conversions * remaining work explained in #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...

jdm · 2021-03-12T21:47:04Z

Fixed in #272

vvuk and others added 30 commits June 24, 2019 11:53

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

cf2ecd9

…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

03687e3

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

1f22aa0

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()

ad1b9ff

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

50fe7c4

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

57bc52e

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

ab99734

windows: cleanup: Use more standard indentation for macro

0e48d3f

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

19af7c9

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

db07779

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

80ee2d7

windows: Mark notify_completion() as unsafe

505a3df

This method could yield uninitialised data when invoked incorrectly.

windows: Widen too narrow unsafe block

27376c3

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

4cc8401

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

1c33a04

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

6642fbe

windows: Take ownership of handle in move_handle_to_process()

5f70a30

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

windows: Assert valid state before consuming receiver

5649048

highfive added the S-needs-rebase label Dec 20, 2019

jdm mentioned this pull request Dec 24, 2019

[WIP] Add native Windows back-end #166

Closed

mitsuhiko mentioned this pull request Feb 12, 2020

Add Windows Support mitsuhiko/procspawn#20

Open

jdm mentioned this pull request Jun 6, 2020

Measure performance impact of Windows IPC on immersive mode servo/servo#26809

Open

bjorn3 mentioned this pull request Aug 10, 2020

Miri stdout is buffered and stdin in always empty rust-lang/miri#1505

Closed

tdgne mentioned this pull request Mar 2, 2021

Windows (rebased and updated) #272

Merged

jdm closed this Mar 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows #233

Windows #233

angelortiz1007 commented Jun 24, 2019

bors-servo commented Dec 20, 2019

ofek commented Dec 24, 2019

sunjay commented May 7, 2020

antrik commented May 8, 2020

jdm commented May 8, 2020

BatmanAoD commented Jul 8, 2020

vvuk commented Jul 8, 2020

Boscop commented Jan 10, 2021 •

edited

Loading

jdm commented Jan 10, 2021

Boscop commented Jan 10, 2021

jdm commented Jan 10, 2021

vvuk commented Jan 11, 2021

jdm commented Jan 11, 2021

vvuk commented Jan 11, 2021

Boscop commented Jan 11, 2021

jdm commented Jan 12, 2021

ofek commented Jan 12, 2021

jdm commented Mar 12, 2021

Windows #233

Windows #233

Conversation

angelortiz1007 commented Jun 24, 2019

bors-servo commented Dec 20, 2019

ofek commented Dec 24, 2019

sunjay commented May 7, 2020

antrik commented May 8, 2020

jdm commented May 8, 2020

BatmanAoD commented Jul 8, 2020

vvuk commented Jul 8, 2020

Boscop commented Jan 10, 2021 • edited Loading

jdm commented Jan 10, 2021

Boscop commented Jan 10, 2021

jdm commented Jan 10, 2021

vvuk commented Jan 11, 2021

jdm commented Jan 11, 2021

vvuk commented Jan 11, 2021

Boscop commented Jan 11, 2021

jdm commented Jan 12, 2021

ofek commented Jan 12, 2021

jdm commented Mar 12, 2021

Boscop commented Jan 10, 2021 •

edited

Loading