Windows: Fallback for overlapped I/O

[ChrisDenton]

Fixes #98947

[rustbot]

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

[wesleywiser]

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

[jstarks]

LGTM

[jstarks]

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

[ChrisDenton]

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.

[jstarks]

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.

[ChrisDenton]

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

[ChrisDenton]

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.

[thomcc]

So... this is pretty subtle. I think it's ultimately fine, but it took a lot of discussion with @ChrisDenton on Discord to convince me (but to be clear: I am on board with it now).

That said, for future reference, this is usually not how you're supposed to do it. For example, the documentation for GetOverlappedResult states:

Use of file, named pipe, or communications-device handles for this purpose is discouraged.

My understanding is the "right" way to solve this is to first call GetOverlappedResult with bWait == FALSE, to reset the event, and then call GetOverlappedResult with bWait == TRUE to actually wait. Failing to do this would result in it not actually waiting, which is what was observed when we used a single GetOverlappedResult(TRUE) call rather than WaitForSingleObject. I believe this was also required prior to Windows 7, and in many cases it is still true, such as when a manual-reset event is used in the OVERLAPPED (although my experience has certainly been that you still sometimes need to do this even if you don't!)

In most cases, I still think that is how it should be handled (notably it is sometimes required, but correct and harmless even in the case where you don't need to go through with it). However, in this case, I think it would actually be pretty weird, and we're justified:

1. We don't actually have an event, we're using the one in the file handle (well, that would often be true either way). At least in theory this means we should not need to do the double GOR dance.
2. We're also not using ReadFile, we're using NtReadFile, and as a result, we don't even have an OVERLAPPED in the first place. We would need to fabricate one with the state we expect it to have after ReadFile which returned pending. This is a bit of an abstraction violation, to say the least.
3. We don't need any of the other information provided by GetOverlappedResult. What we need it to do is wait, so we should just perform the wait.

Anyway, that's all to say I think this is fine. @ChrisDenton has asked me to wait on the R+ for a bit (for a response from @jstarks), so I'll give that a day or so before letting bors know

[jstarks]

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.

[ChrisDenton]

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.

[bors]

📌 Commit 91a6401 has been approved by thomcc

It is now in the queue for this repository.

[Mark-Simulacrum]

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

[bors]

⌛ Testing commit 91a6401 with merge 17355a3...

[bors]

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

[rust-timer]

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

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: 😿 relevant regression found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.1%	2.1%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

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 ↩

[cuviper]

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

rust/library/std/src/fs.rs

Lines 90 to 92 in 23e21bd

	/// On Windows, the implementation of [`Read`] and [`Write`] traits for `File`
	/// perform synchronous I/O operations. Therefore the underlying file must not
	/// have been opened for asynchronous I/O (e.g. by using `FILE_FLAG_OVERLAPPED`).

[ChrisDenton]

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.