Add an IOError when send() is called on a closed connection

[adriangb]

Closes #66

[adriangb]

For websockets the event had additional information, namely the code. Should we specify that the exception MUST have a code attribute?

[adriangb]

cc @andrewgodwin @Kludex

[carltongibson]

Can a raise a strong concern about this proposal.

We use http.disconnect in Django to allow the framework to detect client disconnects on long-running connections, and clean up properly.

We're very happy with it, and this change would be highly disruptive.

It's also not clear how this resolves the issues linked to on the ticket… If I'm running a long running task, I want to listen for http.disconnect on receive() before completing the (presumably expensive) work that leads to a send(). If http.disconnect is removed, there would be no way to cancel the work prior to getting an exception on send(). That's a strict regression.

[adriangb]

You'd still be able to detect a disconnect my catching an IOError raised when send() is called.

It's also not clear how this resolves the issues linked to on the ticket… If I'm running a long running task, I want to listen for http.disconnect on receive() before completing the (presumably expensive) work that leads to a send()

Let me see. I assume you don't care where or how you are notified of the disconnect, but you do want to be notified.
The issue is that as it stands the only way to do that is to call receive(). This is problematic because if your long-running task also wants to consume the request body you're now calling receive() for two different reasons, and unless you can do that in the same place you're going to have issues like Starlette does.

Two options:

• Keep the existing events around forever as a way to get the information by calling receive()
• Raise the exact same IOError when receive() is called

I like the second option. It makes sense to me that any time you try to interact with a client that is disconnected, be it sending or receiving data, you would be notified (via an error) that the client is disconnected. It's then up the user / framework to determine which of the two interactions they use.

[andrewgodwin]

I do not believe we discussed deprecating the disconnect message at all, merely raising an error when someone tries to send() down a closed socket. @adriangb, did I interpret something wrong?

[adriangb]

@andrewgodwin you are right and did not misunderstand, we had not explicitly discussed it. I felt it made sense to make receive() symmetrical with send() in raising an IOError, thus making the disconnect message redundant and a good candidate for deprecation in the next major version of the spec. As it stands right now we'll end up with an asymmetry. But that's not a big deal, I've reverted the wording to only what we explicitly discussed.

[andrewgodwin]

OK, only one more nit now - the spec doc has two empty lines before each header and you have just one. Fix that and we can merge it in, I think.

[adriangb]

Done!

Can we do a minor spec version bump after this so that frameworks can know if the server supports this?

[andrewgodwin]

Yes, I think that would be appropriate - I'll add one in after this is merged.

[MRigal]

@andrewgodwin Isn't an update to this chapter https://asgi.readthedocs.io/en/latest/specs/www.html#disconnect-receive-event-ws also necessary? I find it misleading with the actual context if someone is reading it and now knowing/understanding it derives from https://asgi.readthedocs.io/en/latest/specs/www.html#disconnected-client-send-exception

[andrewgodwin]

No, they are not related to each other - the disconnect event is emitted when you call receive(), and the disconnected error is if you try to call send() once the other side has disconnected.

I'm happy to add more clarification there if you think it's still confusing.

[MRigal]

You're fully right, I've mixed up the things and I needed to give it a fresh look later on to catch up better on it.

I still think that if someone is looking up for infos/specs for WebSockets send events, it's unlikely that this person (if not reading the full page) will get that this could generate a IOError subclass exception.

The structure of the page does not make explicitly clear if the exception applies for both HTTP and WebSockets scope, that could be specified more.

Also (and this is more what my original thought in the first comment was), there could a short sentence/infobox/warning for every description of a send event that this could generate an exception...

[andrewgodwin]

I'll go into the spec and see if I can add a bit of explicit clarification!