Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
njsmith opened this issue Oct 25, 2019 · 0 comments

Comments

@njsmith
Copy link
Member

njsmith commented Oct 25, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants