Windows (rebased and updated) #272

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

…de parameter This is more in line with the other back-ends, as well as general Rust conventions.

…buf` The whole thing is highly unsafe. To handle it *properly*, we would need to encapsulate these values such that nothing else can access them while there is an outstanding async read in progress with the kernel... However, that's quite tricky to achieve -- so for now, the best we can do is putting big fat warnings everywhere, and adding extra safety checks where possible.

Now that `get_message()` has a check to make sure it won't touch the buffer if an async read is in progress, the safety of the invocation is no longer affected by the correctness of the other code in the unsafe block, and vice versa.

…write_buf()` I see no reason to use the raw handle here -- so let's stick with the safer abstraction...

Use a single function with a mode flag, rather than maintaining two separate, nearly identical functions.

Sub-slicing is a safe operation (always yielding a valid slice) -- so it shouldn't affect the soundness of the actual unsafe code.

While it's an unlikely case, I don't see any problem with the buffer being used up to the last byte...

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

Now that nothing touches the `ov` field outside the implementation of `MessageReader`, it makes much more sense to make `MessageReader` responsible for upholding the `Send` property.

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

…se sites Extract the raw handle from the `WinHandle` structure individually wherever it's actually needed for the Windows API calls, rather than doing it up front at the beginning of the method. Dealing with the raw handles as locally as possible seems more robust; and it's also more in line with what we do elsewhere.

The other arm is empty; and since this is on an `Option<>`, we do not benefit from the exhaustiveness check either -- so the more compact variant seems preferable.

This is related to the fix for the `inprocess` back-end in 750d9a1 : according to the documentation of `ErrorKind`, just like the related Unix error, `ErrorKind::BrokenPipe` is supposed to signal only a "receiver closed" condition -- and not "sender closed". Note that the situation is quite confusing here, since the Windows API actually returns a `winapi::ERROR_BROKEN_PIPE` for the "sender closed" condition; so it would seem an obvious choice to turn it into `ErrorKind::BrokenPipe`... Except that this conflicts with the stated purpose according to the documentation. Rather than explicitly deciding on the right `ErrorKind` to use here, we just punt this back to the Windows API, letting it assign whatever `ErrorKind` it deems appropriate for this situation -- thus avoiding a potential confusing discrepancy with other `winapi` users, at the cost of a potential confusing discrepancy with other `ipc-channel` back-ends... The situation here differs from that in the `inprocess` back-end also in that we were already using a distinct `WinError::ChannelClosed` internal state for closed senders, rather than throwing it in with the handling of closed receivers -- so it didn't affect the behaviour of the public `channel_is_closed()` method here, but only the converted `Error` value. (And thus the message printed on `unwrap()`.)

More idiomatic code is much shorter, and probably easier to follow too.

Check for the "closed" status right after issuing operations that can set it, rather than only after draining pending messages. This makes the control flow much clearer in general; and it also avoids suggesting that we could have both pending messages and closed status at the same time -- which is not the case. The more straightforward handling flow will also facilitate further refactoring. Note that there is one place where we still do not handle the closed status immediately: if we get a "closed" notification while starting a read in the course of adding a receiver to a set, we actually have to delay reporting the status until the next `select()` call. (Though the check there still happens before trying to drain messages -- even before trying to fetch the completion status, in fact.) We also do not immediately handle the "closed" status when re-initiating the async read after receiving data inside `select()`; keeping the original approach instead for now, where it gets handled as a side effect of the code for the case described above. We aren't changing this bit for now, since returning the "closed" event immediately (rather than delaying to the next `select()` call) will actually be a slight behaviour change, as opposed to just pure refactoring.

Immediately handle the `closed` status -- rather than postponing handling to the next `select()` call -- even if it gets set in `start_read()`. (It already was being reported immediately in case the status got set in `notify_completion()`.) This is undoubtedly the more obvious and desirable behaviour. (I wonder whether we should actually enforce it with a test case?...) Also, the code should be easier to follow this way.

Only use the indirection with the `MessageReader.closed` flag where it is really necessary, i.e. where we aren't directly passing the notification to the caller, but rather need to delay it. Specifically, this only happens when getting a "closed" notification while adding a receiver to a set. In all other cases, just work with a regular return status. This simplifies the code in some places, and complicated it in others -- but in *all* cases, the more direct handling should make the code easier to follow...

…closed` flag Since the `closed` flag was now only being used to signal a reader that has received a "closed" notification while being added to a set, the set can just keep such closed readers in a dedicated vector instead, thus avoiding the need for the `closed` flag altogether. This makes the code simpler, clearer, and less error prone. There is a slight behaviour change resulting from this: the `MessageReader` object of the closed reader is now dropped already while the reader is being added to the set, rather than only on the next `select()` call. This shouldn't have any observable effect, though.

This makes it more explicit that we are getting a raw handle back rather than another `WinHandle`; and avoids confusion with the ordinary meaning of `take()`, which preserves the type.

Drop unnecessary explicit `iter()` calls; and don't use reference iterators when we can (and indeed should) consume the values.

There was a bunch of places that used raw handles, although using the `WinHandle` wrapper works perfectly fine, and there is no reason to forsake the additional guarantees provided by the type system. This slightly complicates the code in some places, while somewhat simplifying it in others -- it pretty much feels cleaner across the board, though. As a side effect, a bunch of functions are not marked `unsafe` anymore, since they were only marked as such (somewhat controversially) for their use of raw handles.

…aning Now that `WinHandle` is used in more places, many previous uses of `take_raw()` now actually benefit from a proper `take()` method, returning another `WinHandle` while invalidating the original one.

The use of dereferencing to get at the raw handle always felt unintuitive to me, and hard to keep track of. Using a more explicit `as_raw()` method instead seems clearer. (It should also be more flexible I think -- though we aren't actually making use of any of the added possibilities...)

Do the type casting in a different place, which makes it quite trivial compared to the somewhat icky pointer conversion needed before.

When receiving an event for a reader, instead of keeping it in the list, and only removing it at the end if it got closed, we now remove it immediately, only to add it back later when a new read is started successfully. This simplifies the code a bit; and it should be more robust too, since a reader's presence in the list is now tied more closely to it having a read in flight, i.e. being able to receive events.

Correct IOCP event handling is critical to the soundness of the `select()` code: mixing it up might result in trying to access a buffer and `OVERLAPPED` structure that the kernel is not actually done with yet... This means that all code on the path from receiving IOCP events, to changing the status of the corresponding readers in `notify_completion()`, needs to be considered unsafe. Handling the results of `notify_completion()` and `start_read()` on the other hand isn't unsafe at all, since neither affects the individual readers' `read_in_progress` flags -- which have sole responsibility for keeping track of kernel aliasing. The worst that could happen here is trying to fetch messages from the a reader which didn't actually receive (which is begnign), and/or losing track of which readers are still open.

`set_id` kept misleading me, sounding like it's the ID of the set the receiver is part of, rather than the ID of the receiver within the set...

Initialise using exhaustive conditionals, avoiding mutation.

Rather than passing a naked error code (with a magic value denoting success), use a standard `Result<>` enum.

Use exhaustive match in one place rather than scattered conditionals. This should be much easier to follow.

Set the flag before issuing the system call, and only reset it later if it turns out no read was actually started. This way it's less likely that setting the flag gets ommited by mistake, which could have catastrophic effects, since setting this is crucial for tracking the kernel aliasing of `ov` and `read_buf`.

This field is only meaningful while an async read is in progress with the kernel; and we never reuse it between reads. Making this more explicit and robust by only giving it a value while it's in use.

Since the `ov` field is now only set when we have a read in progress, and thus effectively serves as an indicator of that state, the explicit flag became redundant: we can just check the presence of `ov` instead. (And since a check is performed implicitly when unwrapping the `ov` value, some of the explicit asserts on `read_in_progress` can be dropped entirely.)

Introduce an `AliasedCell` wrapper type for the fields that get aliased by the kernel during async read operations, making sure that these values can only be accessed through special unsafe methods, i.e. only inside `unsafe` blocks, even if the wrappers get passed around through safe code. This makes invoking `MessageReader.start_read()` safe; and consequently removes all unsafety from `OsIpcReceiver`. (`OsIpcReceiverSet.select()` retains some unsafe code though, since it does raw I/O itself... This can only be fixed by factoring out that code.)

Put both of the fields aliased by the kernel during an async read operation together in a common `AliasedCell<>`. This increases robustness, and further tightens unsafe boundaries, by making sure the two fields stay consistent with each other. When they are wrapped in `AliasedCell<>` separately, safe code is prevented from giving any of them invalid values individually -- but they could still get out of sync with each other, if some code moves just one of them and not the other. Consistency between these fields however is crucial for correctness as well as soundness.

While this might have been a problem in the past, the current code properly passes errors from `notify_completion()` up through all layers; so there is nothing really preventing us from orderly returning any kind of error reported by the `GetOverlappedResult()` or `GetQueuedCompletionStatus()` system calls. (The only problem would be if `GetOverlappedResult()` has failure modes that actually leave the async operation in progress, and thus the async data in use: in that case, unpacking the `AliasedCell<>` in async and returning to the caller would be wrong... But if that's the case, the previous behaviour of panicking after unpacking the `AliasedCell<>` was just as wrong.) Since returning arbitrary errors requires us to invoke `WinError::from_system()` (either directly, or indirectly through `WinError::last()`), and this one wants to know the origin of the error (so it can put it in debug traces), we need to do these conversions near the call sites, and pass an already converted error to `notify_completion()`. (Which seems cleaner anyway.) This has the side effect of also logging "broken pipe" (sender closed) errors in the debug trace, which might be slightly redundant with any other tracing done by the "closed" handling... I don't think that's really a problem, though.

Split out the system call and associated handling for getting completion notifications on an IOCP (set) from the rest of the `select()` method, similar to how `fetch_async_result()` encapsulates the event handling for regular readers. This will be necessary for proper `Drop` handling for `OsIpcReceiverSet`. Incidentally, this exactly covers tha unsafe code section of the `select()` implementation; and as such, it's a good step towards better layering of the IOCP handling in general. I guess it could be argued that a method call is also more readable than assigning from a large anonymous block...

According to the documentation of `CancelIoEx()`, successful completion of the `CancelIoEx()` call only indicates that the cancel request has been successfully *queued* -- but it does *not* mean we can safely free the aliased buffers yet! Rather, we have to wait for a notification signalling the completion of the async operation itself. We thus split out the actual `CancelIoEx()` call into a new `issue_async_cancel()` method, and turn `cancel_io()` into a wrapper that waits for the actualy async read to conclude (using `fetch_async_result()`) after issuing the cancel request. Since that doesn't work on readers in a receiver set, we need to add an explicit `Drop` implementation for `OsIpcReceiverSet`, which issues cancel requests for all outstanding read operations, and then uses `fetch_iocp_result()` to wait for all of them to conclude.

Increase chance of catching intermittent failures while working on other stuff...

Make sure all code related to the `win32-trace` feature is compiled only when the feature is enabled. This avoids unnecessary bloat; as well as potential compile errors when other code conditional on this feature is added.

Add a (conditional) helper method for obtaining the raw handle of the reader -- which is often needed for the `win32_trace` invocations -- to abstract the internal structure of this type, thus facilitating further refactoring. An alternate approach would be overriding the `Debug` trait on `MessageReader`, to just print the raw handle value. That would provide better encapsulation; however, it would also preclude the possibility of easily printing all the constituents of the structure during debugging... (Or we could leave `Debug` alone, and instead implement it as `Display` -- but that feels like an abuse of the `Display` facility... Not sure what to think about that.)

For the duration of an async read operation, move the pipe handle into the `AliasedCell` along with the other fields used for the async operation. This prevents anything else from messing with the pipe while the async read is in progress; and makes sure the handle and the other fields can never get mismatched. While I'm not sure whether there is any scenario in which such a mismatch could result in undefined behaviour, it's good for general robustness in any case.

…() apis to verify if self.h and other.h handles point to the same object.

…ck-end" This reverts commit 2f2b158.

This reverts commit 3facdd3.

…lity` is absent

…bal ROUTER

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows (rebased and updated) #272

Windows (rebased and updated) #272

Commits on Feb 27, 2021

Commits on Mar 2, 2021

Commits on Mar 9, 2021