Fixed socket aclose() delayed close, missing checkpoint and Windows hang

[graingert]

NOTE Erasing or replacing the contents of this template will result in your pull
request being summarily closed without consideration!

Changes

Fixes #1237. Fixes #1273. Fixes #1288.

Socket aclose() implementations had three related problems.

On the asyncio backend, SocketStream, UDPSocket and ConnectedUDPSocket closed
through transport.close(), which only schedules the FD-closing connection_lost
callback via loop.call_soon. A second, concurrent aclose() — as
aclose_forcefully() can produce — therefore returned while the socket was still open,
and on Windows closing a UDP socket with a datagram still in flight could hang outright.
All three now abort() the transport directly, which also makes
DatagramProtocol.closed_event unnecessary.

None of the implementations checkpointed either, so aclose() called inside an
already-cancelled scope returned normally instead of raising a cancellation exception.
This affected UNIX socket streams and UNIX datagram sockets on asyncio, and every socket
type on Trio.

_RawSocketMixin.aclose() now always closes the raw socket and checkpoints, while still
resolving pending receive and send futures only on the first call, so a task blocked in
receive() or send() is woken exactly once.

Checklist

If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):

• You've added tests (in tests/) which would fail without your patch
• You've updated the documentation (in docs/), in case of behavior changes or new
  features
• You've added a new changelog entry (in docs/versionhistory.rst).

If this is a trivial change, like a typo fix or a code reformatting, then you can ignore
these instructions.

[agronholm]

Does this obsolete #1273?
EDIT: Oops, you included that in the description, never mind.

[graingert]

No it fixes it, #1273 is an issue, did you mean to reference a PR?

[agronholm]

Sorry, I'm juggling a number of related PRs and issues in the milestone.

[agronholm]

So, my first thought here was that we checkpoint in aclose() after the close operation has happened. Usually, if we enter a coroutine function, we would normally checkpoint before the actual changes, to allow for cancellation to take effect, but here we're only checkpointing after the fact. Granted, we didn't checkpoint at all previously, so it's not a regression, but this seems to be in stark contrast with the usual cancellation semantics. Thoughts?

[graingert]

see https://trio.readthedocs.io/en/stable/reference-core.html#cancellation-and-primitive-operations

Async cleanup operations – like __aexit__ methods or async close methods – are cancellable just like anything else except that if they are cancelled, they still perform a minimum level of cleanup before raising Cancelled.

[graingert]

and https://trio.readthedocs.io/en/stable/reference-io.html#trio.abc.AsyncResource.aclose

[agronholm]

and https://trio.readthedocs.io/en/stable/reference-io.html#trio.abc.AsyncResource.aclose

OK, makes sense.

[agronholm]

No it fixes it, #1273 is an issue, did you mean to reference a PR?

Right, so I was actually thinking about #1246, #1255 and #1277. Got the numbers mixed up.

[agronholm]

I have concerns about the abort(). According to the docs, it basically force-closes the connection without waiting for pending operations to finish. When force-closing the stream, that's fine, but aclose() is also called under normal closing circumstances. Wouldn't we potentially lose data if we did this?

[graingert]

Because we call set_write_buffer_limits(0) once SocketStream.send returns there's no pending operations. If we call aclose concurrently to send only then do we lose data, but that's what Trio does too