Windows: Fallback for overlapped I/O by ChrisDenton · Pull Request #98950 · rust...

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

cc @jstarks in case you want to take a look as well

Might be good to add a test, perhaps by creating (and connecting to) an overlapped named pipe and reading from it.

I added a test (note that the anon_pipe function actually creates named pipes, one end of which is OVERLAPPED). On my machine I confirmed it tests the new code path by replacing io_pending_fallback with a panic.

Thanks.

On a side note, I am trying to get NtCreateNamedPipeFile documented, which would allow you to create an anonymous pipe pair with the properties you want without the retry dance. I'll let you know when this happens.

@jstarks ah, it turns out GetOverlappedResult is sometimes returning success but the status is still pending. Might it be better to use WaitForSingleObject directly and always check the IO_STATUS_BLOCK status?

I'm using this test (dbe0e28) and it's triggering your debug line.

Ok, from what I can tell via observer behaviour, GetOverlappedResult calls WaitForSingleObject and then assumes that if it returns success then the I/O has completed. However, this will not be true if the file handle was signalled for some other reason. Thus using GetOverlappedResult does not fix the soundness issue in the original issue.

However, with this information I can use WaitForSingleObject directly and make sure the I/O has actually completed before returning (or else aborting if that's not possible). Which is what I've done in the new commits.

Wow, I did not expect that wait behavior out of GetOverlappedResult, but you're absolutely right, the code does not check for STATUS_PENDING unless you specify Wait = FALSE. Yikes!

The new approach is a good one, both for this reason and to avoid the abstraction violation. Thanks @ChrisDenton and @thomcc.

Yeah, I find that really surprising. At first I was convinced I'd made a mistake somewhere. I guess it really is intended to be used with a dedicated event object.

Alright, let's go!

@bors r+

Commit 91a6401 has been approved by thomcc

It is now in the queue for this repository.

@bors p=1 since we're targeting beta backport

Testing commit 91a6401 with merge 17355a3...

Test successful - checks-actions
Approved by: thomcc
Pushing 17355a3 to master...

Finished benchmarking commit (17355a3): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

Cycles

This benchmark run did not return any relevant results for this metric.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. the arithmetic mean of the percent change

2. number of relevant changes

The documentation on File still forbids FILE_FLAG_OVERLAPPED -- should that be updated?

Personally I think we should still warn against using files opened for async I/O here. This PR may help mitigate the issue if users are careful (esp. with regard to threads) but these methods are still not intended for async I/O.

I guess the "must" language could be made slightly weaker. But this PR is purely a "best effort" attempt at a workaround in that case so I do feel we should still strongly suggest people use other methods for async I/O.