python-trio / python-trio/trio

On windows, notify_closing should accept arbitrary handles and wake up other handle operations

Open
#1,270 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

low-level Windows
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_overlapped
  • write_overlapped
  • readinto_overlapped
  • WaitForSingleObject

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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.