python-trio / python-trio/trio
On windows, notify_closing should accept arbitrary handles and wake up other handle operations
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 7.3k
- Forks
- 431
- Avg merge
- 2d 17h
- Merged PRs (30d)
- 6
Description
On Windows, we expose a number of operations that take handles:
wait_readable(SOCKET handles only)wait_writable(SOCKET handles only)wait_overlappedwrite_overlappedreadinto_overlappedWaitForSingleObject
Currently, when you call notify_closing(handle), we require that handle must be a SOCKET, and it only wakes up wait_readable and wait_writable calls. It would make more sense if it accepted arbitrary handles, and caused all of these operations to immediately exit with ClosedResourceError.
This is probably a precondition to #824. Right now our named pipes notice when the handle got closed out from under them, but the way they do this is by relying on the kernel notifying that the underlying object got closed. In general, though, handles can be dup'ed, and then closing the handle no longer closes the underlying object, and I think that would break us. It's OK for now because we use named pipes in such a limited way that we know there's exactly one handle to them, but I don't think it works in general.
I don't think fixing this would be terribly difficult...
Both write_overlapped and readinto_overlapped use wait_overlapped under the hood, so if we fix wait_overlapped then we automatically fix all three. And wait_overlapped already assumes that if the operation exits with ERROR_OPERATION_ABORTED and no cancellation was requested, then the handle must have been closed. So if we kept a table mapping handle→wait_overlapped, and notify_closing called CancelIoEx on all those operations, then that might be enough by itself?
We would want to think carefully about the whole posted_too_late_to_cancel system and what would happen if the same operation got cancelled twice, once via notify_closing and once via the abort fn. Maybe we should use custom_sleep_data to track whether we've called CancelIoEx, so that notify_closing and the abort fn could coordinate and whoever runs second skips calling CancelIoEx?
For WaitForSingleObject, it's a bit annoying because it lives outside of trio/_core, so we'd need to expose some new API for it. Maybe with trio.hazmat.raise_if_handle_closed(handle): ..., which is implemented like:
@contextmanager
def raise_if_handle_closed(self, handle):
with CancelScope() as cscope:
self._cancel_by_handle[handle].add(cscope)
try:
yield
finally:
self._cancel_by_handle[handle].remove(cscope)
if cscope.cancelled_caught:
raise ClosedResourceError
I guess we could also use this inside wait_overlapped... it's a bit heavy-weight maybe, which isn't an issue for WaitForSingleObject which uses threads, but maybe we want something cheaper for wait_overlapped? OTOH maybe that's premature optimization.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by tracing the Windows implementations of notify_closing and wait_overlapped in trio/_core, then inspect how WaitForSingleObject exposes handle waits outside trio/_core. Review the cancellation bookkeeping, including posted_too_late_to_cancel and custom_sleep_data, before choosing the coordination approach. Done means arbitrary handles can be notified and all listed operations exit with ClosedResourceError.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- operating-systems
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100