Add file handle support for overlapped I/O

[klu-dev]

Closes #97

This is an implementation to support Windows file handle overlapped I/O. The basic idea is:

1. Add an File struct in PacketInner. The File struct have read and write OVERLAPPED struct. The read and write struct address will exposed to callers. The caller use read and write overlapped pointer as parameter for ReadFile/WriteFile operation.
2. IOCP completion events receive read and write address which will be converted back to Packet for updating events
3. New PollerIocpFileExt trait which has add_file and remove_file API to extend file functionality.

[notgull]

Thank you! I imagine this was a significant effort. I appreciate you writing this. However, I definitely need to read through this and understand this. So this review may take some time.

The immediate thing that jumps out at me is the weird pointer offsets in the FileOverlapped traits. This seems fragile to me. I would prefer them to be direct pointers; I think that would be better when it comes to strict provenance.

Also, have you run this code under Miri on Windows?

[klu-dev]

Hi @notgull

Thanks for reviewing. FileCompletionHandle trait would provide conversion from overlapped pointer to packet which will depends on FileOverlapped trait providing pointer offset to packet. Maybe should rename FileOverlapped to FileOverlappedOffset.

I do not run the code with miri. Currently I am writing NamedPipe code in async_io crate. I found the code need some change in order for better integration with async_io. I also found a problem that is IOCP port still can receive event after the file handle is removed from the poller and file handle is closed as long as there is I/O operation with overlapped pointer. But the packet has been dropped at this time. This will cause memory access violation

I will request review after the issue is fixed. I will also run code in miri and solve your comments.

[klu-dev]

Hi @notgull

I have updated the code to solve IocpFilePacket returned by add_file lifetime issue. The idea is IocpFilePacket will increase reference count of Packet, so the Pakcet is still valid when the Poller is dropped. And for every success overlapped I/O operation ((the operation return TRUE or ERROR_IO_PENDING) will also add reference count, the reference count will decrease after the IOCP event is received. This will make sure the IOCP returned overlapped pointer still valid even the packet is remove from the poller and IocpFilePacket is dropped. I also update the code according your previous comments.

I implmeneted NamedPipe in async-io according the updated polling code. The test code is https://github.com/klu-dev/async-io/blob/kail/windows_named_pipe/tests/windows_named_pipe.rs . The main function has completed. I am thinking adding MESSAGE mode and configurable parameter when create named pipe.

I try to run miri, but get the following error. Miri do not support code calling native OS API. So the code add an IocpFilePacket::test_ref_count method used in test case to check Packet Arc count.

test writable_after_register ... error: unsupported operation: CreateFileW: Unsupported flags_and_attributes: 1073741824
--> C:\Users\tom_k.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\sys\fs\windows.rs:334:13
|
334 | / c::CreateFileW(
335 | | path.as_ptr(),
336 | | opts.get_access_mode()?,
337 | | opts.share_mode,
... |
341 | | ptr::null_mut(),
342 | | )
| |_____________^ unsupported operation occurred here
|
= help: this is likely not a bug in the program; it indicates that the program performed an operation that Miri does not support
= note: BACKTRACE on thread writable_after_register:
= note: inside std::sys::fs::windows::File::open_native at C:\Users\tom_k.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\sys\fs\windows.rs:334:13: 342:14
= note: inside std::sys::fs::windows::File::open at C:\Users\tom_k.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\sys\fs\windows.rs:323:9: 323:39
= note: inside std::fs::OpenOptions::_open at C:\Users\tom_k.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\fs.rs:1721:9: 1721:42
= note: inside std::fs::OpenOptions::open::<&str> at C:\Users\tom_k.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\fs.rs:1717:9: 1717:34
note: inside client
--> tests\windows_overlapped.rs:197:16
|
197 | let file = opts.open(name)?;
| ^^^^^^^^^^^^^^^
note: inside writable_after_register
--> tests\windows_overlapped.rs:216:22
|
216 | let client = client(&name);
| ^^^^^^^^^^^^^
note: inside closure
--> tests\windows_overlapped.rs:213:29

[notgull]

Noting that I've seen this patch, I'm doing research to ensure everything is sound.

[arnodb]

Hi, first of all thanks for the hard work!

I don't know if this PR is still on the radar, but I am experimenting with it and the async-io paired branch. My code is located at:

https://github.com/arnodb/teleop/tree/windows-named-pipe

My results so far are visible at https://github.com/arnodb/teleop/actions/runs/21923871658 (or more recent runs of CI on the branch).

Surprisingly the unit test attach::windows_named_pipe::tests::test_named_pipe_attachment passes as opposed to the real life example which:

• seems to receive multiple client connections (according to the debug I added)
• seems to panic in the client (Task polled after completion) but it may be cause by another error I am not aware of (it could also be an issue in async-io when things go wrong)

I hope this use case can help making progress on overlapped I/O in polling and async-io.

[arnodb]

@klu-dev, Task polled after completion is simply a self.flush = None; missing in poll_flush (async-io branch). I'm now investigating other issues.

[klu-dev]

@arnodb Sorry for reply lately. I am looking at the issue. The crash comes from the poll_flush can be fixed easily. But I found another issue for implementation in async-io about windows named pipe. The problem is named pipe events are edge triggered. The event is only triggered once, so current implmentation does not support poll many times like select! macro in the server.rs in teleop. I am trying to fix this issue.

[klu-dev]

@arnodb The issue has been fixed in klu-dev/async-io@fd02db0 . async-io repo is updated, but there is no code change for polling repo. I found another issue for your server.rs code, the future stream = futures::StreamExt::next(&mut conn_stream).fuse() => will frequently be polled after there is client connection to server. The reason task runnner LocalPool do not wake up specific task independtly, instead it wake up the thread, and loop all task for poll, which cause any task task wake up will cause all tasks polled once.

async-io NamedPipeListener::bind() add PipeMode and NamedPipeOpenOptions parameters to support set pipe params. Your code need to change to NamedPipeListener::bind(named_pipe_name(std::process::id()), Default::default(), None)?; in windows_named_pipe.rs::listen code.

[arnodb]

Hi @klu-dev, thanks for this update!

I pushed (force) the latest changes to my branch. It looks like it works now 🎉.

Regarding the LocalPool executor comment, I assume this is not an issue in my code strictly speaking but a tradeoff of the executor itself.

If I understand your comment correctly: all the tasks in the executor are polled whenever there is activity in one of them (e.g. a byte sent through the wire), correct?

I didn't know that (still new to async details), thanks. Is it an issue? I guess generally "not really", but in combination with your named pipes? Did you notice problematic behaviour?

(Good news: teleop does not force anyone to use LocalPool 😅)

[klu-dev]

@arnodb I found the root cause which is poll_flush bug in async-io for named pipe. It is fixed in commit klu-dev/async-io@10126a1.

[notgull]

Sorry for the delay; this is large PR and I haven't found the time to sit down and review the entire thing in one sitting.

[klu-dev]

@notgull I update the code based on AI code review. The code improvement could refer to section 2 and 3 in docs/named-pipe.design.md doc