Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upHbt win2 #184
Closed
Hbt win2 #184
+1,848
−11
Conversation
Since the `fork()`-related code is not compiled on Windows platforms, the related `use` statements need to be conditional too.
…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.
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.
Again, while it's an unlikely case, I see no problem with fully using the available value range.
The only really unsafe part here is turning the `MessageHeader` struct into a series of bytes. All the actual buffer manipulations can be done just as efficiently using safe operations. This includes a major revamp of the way we deal with the header construction: making it more straightforward, and reusing existing functionality.
Abstracting this trivial functionality only makes it more obscure.
Tuple structs are obscure, and are used pretty much only for the "newtype" pattern -- a struct with named fields is generally more readable.
Adaptation of same mechanism in unix back-end.
That code resulted from some kind of misunderstanding...
This way we can do simple comparision, rather than needing one-arm match statements.
…unction The convention is to keep all `impl`s of a type together in one place.
Should be easier to read than mixing explicit and implicit return for no obvious reason...
Since the `Ok` and `ERROR_IO_PENDING` cases are identical (as explained in the comment), just fall through to a common default handler.
Put the success case before the error cases, rather than in between...
Further streamline error handling by turning the FFI result into a `Result<>` value, so we can handle it in one uniform `match` expression. While it could be argue that this adds unnecessary extra code, I believe it makes the error handling easier to follow -- especially the common handling of the `Ok` and `ERROR_IO_PENDING` cases. It also allows moving the comment closer to the actual case it explains...
The second paragraph explains the same thing as the first one, only clearer... So we can just get rid of the first one entirely.
Apparently this ended up in the wrong place during some edit... While at it, also improve wording.
Don't know whether there was a point to it in some earlier iteration -- but in the current code, this method never returns anything else than `Ok(())`...
This seems to be a leftover from some earlier version of the code...
Move the raw FFI call from `OsIpcReceiver.get_message()` to a new helper method in `MessageReader`. This fixes a major layering violation, and makes the functionality more reusable. Note: this doesn't address the related raw FFI call in `OsIpcReceiverSet.select()` -- that one is a whole other can of worms, which will require a larger refactoring to fix...
The reader is not supposed to be used for anything else; and the caller drops it immediately afterwards anyway. Taking it by value (and dropping it ourself) makes this more explicit.
As far as I can tell, this method only differs from the regular receive pattern in that it allocates the entire buffer for the message of known size in advance, and then reads into it repeatedly until the expected size is filled; at which point it returns the entire buffer -- rather than trying to extract individual messages from the buffer with `get_message()` between reads. Since the actual async read initiation and completion is pretty much the same -- apart from some shortcuts, which should be insignificant I believe -- we can just implement this functionality in terms of the regular methods, rather than keeping separate code paths that do pretty much the same.
Just as with `write_buf()`, I see no reason to use the raw handle -- so let's stick with the safer abstraction here as well...
The local variable isn't moving anywhere -- so no need to put it on the heap for a stable address...
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon. |
|
|
|
sorry for this noise. thought I was in a different fork. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
mwrock commentedDec 21, 2017
No description provided.