Remove undocumented public API usage of UnboundedQueue

[lojack5]

Long time lurker here, I was tinkering around with using IOCP on Windows (messing with ReadDirectoryChanges), dug into some of the undocumented public methods here, as well as the deprecated warning going along with its usage of UnboundedQueue.

So: I updated it to use channels instead, (as well as the kqueue IOManager version). I have a few questions though:

1. The kqueue IOManager doesn't appear to have any tests for the similar API (monitor_kevent). Should one be written? And if so maybe some guidance on tests, I'm pretty new to those, and don't have the appropriate machine setup ATM. This also seems to be why coverage dropped?
2. My second commit adds a synchronous close() method to channels (does the exact same thing, just never awaits a checkpoint). Not sure if this is a thing that makes sense to have or not, but it seemed like it was something that should be used to close the send side of the channel when the IOManager doesn't need it anymore. Maybe there's a better way (queue the aclose() version in the scheduler?).
3. This changes a public API (monitor_completion_key and monitor_kevent yield MemoryRecieveChannels instead of UnboundedQueues). But the API was undocumented to begin with...should I add documentation? Should a news fragment go with this since someone may have been relying on the old interface? And if so, should this wait until UnboundedQueue goes from deprecated to removed?
4. As far as I can tell UnboundedQueue isn't used anywhere else after these changes (I know, not a question).

This is pretty much just to squash the deprecation warning, as I tinker around I'll have more things I might want to change (ex: the context manager for monitor_completion_key is cumbersome to use some cases, though that could just be cause I'm designing my classes wrong for the API).

[codecov]

Codecov Report

Merging #937 into master will decrease coverage by 0.01%.
The diff coverage is 78.12%.

@@            Coverage Diff             @@
##           master     #937      +/-   ##
==========================================
- Coverage   99.39%   99.37%   -0.02%     
==========================================
  Files         102      102              
  Lines       12461    12466       +5     
  Branches      915      913       -2     
==========================================
+ Hits        12385    12388       +3     
- Misses         56       58       +2     
  Partials       20       20

Impacted Files	Coverage Δ
trio/_core/_io_windows.py	88.18% <100%> (+0.1%)	⬆️
trio/_channel.py	100% <100%> (ø)	⬆️
trio/_core/tests/test_windows.py	100% <100%> (ø)	⬆️
trio/_core/_io_kqueue.py	73.58% <22.22%> (-0.93%)	⬇️

[njsmith]

Hey, welcome!

So here's the deal with those low-level APIs: I stuck them in as placeholders when I was first figuring out how Trio's IO should work at all, as a kind of "statement of intent". But then no-one has actually used them so we haven't taken the time to figure out how they should actually work :-). If you want to pick that up, then that'd be awesome!

There's some discussion in #26, #578, #579, especially this comment: #578 (comment)

Those are just random thoughts at different times though, not like, Pronouncements On The Proper Way To Do Things.

I guess the first question is, what is even useful. For Windows, we definitely want a way to handle the super-common pattern of "calling a function that takes an Overlapped". The register_with_iocp and wait_overlapped functions do currently allow that. Are they the nicest way? Not sure, they haven't been used a lot. (I think this is what you need for ReadDirectoryChanges?)

Do we also need a way to monitor a completion key? We have one because I was messing around with PostQueuedCompletionStatus, but I don't know if it's actually useful in general. Ditto with the current_iocp function.

There are similar questions for kevent.

And if we do want the monitor functions, then ... there's a few different ways to do it. We could use a custom iterable, a channel like in this PR, pass in a channel from outside (which allows multiple streams to be monitored on a single channel, and means we don't have to close it)... probably this would be easier to answer if we knew what the purpose of these monitor functions was :-)

Do you have any thoughts?

[lojack5]

So my first attempt was in using register_with_iocp and wait_overlapped. From what I know now I think I could get that to work fine, but here's where waiting on a completion key helped me out a lot:

1. I'm new to IOCP, so I'm not 100% sure what will work, and been decyphering MSDN pages for structs, calls, etc. Because of this, I found it extremely useful to get the dwNumberOfBytesTransferrred after the IOCP completes, which you need to wait on the completion key to get. This really helped me to make have a second check in there to make sure I wasn't reading past actual data in my buffer into uninitialized data in my buffer.
2. Now that I've learned a bit more though, especially reading here, I think I can get it safely working (for ReadDirectoryChanges) by just checking for NextEntryOffset == 0, and relying on IOCP only giving a result if one or more FILE_NOTIFY_INFORMATION structs have been written to the buffer...I think. There might be some weird cases with error codes.

---

So with the above and responding to the rest of your thoughts:

1. Definitely need register_with_iocp and wait_overlapped. Currently I think the interface is fine with those.
2. It'd be nice to be able to also wait on completion keys, though the interface there can use a overhaul (I'll try to think up a good seeming way to do that based on what I had to work with).
3. current_iocp currently is needed if you want to wait on completion keys, since the user has to call CreateIoCompletionPort themselves. This could probably be avoided by handling 2 above.
4. kevent stuff I have no knowledge of and don't have much experience with the platforms involved either.
5. I'll reread the linked issues to see if it sparks more ideas!

Thanks! I'll try to get back within a week with at least a little bit of the above.

[njsmith]

I'm new to IOCP, so I'm not 100% sure what will work, and been decyphering MSDN pages for structs, calls, etc. Because of this, I found it extremely useful to get the dwNumberOfBytesTransferrred after the IOCP completes, which you need to wait on the completion key to get. This really helped me to make have a second check in there to make sure I wasn't reading past actual data in my buffer into uninitialized data in my buffer.

Oh good point! We should make wait_overlapped return the stuff in OVERLAPPED_ENTRY, including the dwNumberOfBytesTransferred. We have the info, no reason to hide it from users.

[lojack5]

Ok, after toying around with this I think going with making wait_overlapped will be a less intrusive change and I think it'll cover all the cases I'd want to use it for. I'll go ahead an close this PR, and I've already implemented a test change on my fork, I'll tinker a bit more, then open a new PR once I'm happy with it.

[njsmith]

Sounds good!