SEP-1699: Support SSE polling via server-side disconnect #1783
Conversation
mikekistler
left a comment
mikekistler
left a comment
There was a problem hiding this comment.
I fully support this change but the devil is in the details. I would like a little more clarity on how we expect servers to implement this and the implications on support of resumable streams.
| support both these cases. | ||
| 6. If the server initiates an SSE stream: | ||
| - The SSE stream **SHOULD** eventually include JSON-RPC _response_ for the | ||
| - The server **MUST** immediately send an SSE event consisting of an event ID |
There was a problem hiding this comment.
Is it the intention of this SEP that all servers now MUST support resumable streams? I think that is the implication of requiring the server to send an initial event with an id field. That is a very significant new burden on servers, and I think it should instead be optional (e.g. weaken this statement to SHOULD).
Or maybe we use this opportunity to clarify the behavior of resumable streams. The current spec is not clear about what behavior is allowed for GET with Last-Event-Id. The only thing disallowed seems to be delivery of messages that would have been delivered on a different stream. Does this mean that it is allowed to simply resume the stream without replaying any previously sent (or attempted to be sent) messages from the stream? Clarifying the language on this point could make resumable streams a much more reasonable burden on servers.
There was a problem hiding this comment.
Is it the intention of this SEP that all servers now MUST support resumable streams? I think that is the implication of requiring the server to send an initial event with an
idfield. That is a very significant new burden on servers, and I think it should instead be optional (e.g. weaken this statement to SHOULD).
No, that is not the intent.
As I was creating this PR, I was on the fence about making this a "SHOULD" (vs a "MUST"). I went with "MUST" because that is in the original SEP and because the burden of just sending an initial event is not high. However, I agree it isn't necessary for servers that don't support resumability, so I will change this to "SHOULD". (Note that the next point — "After the server has sent an SSE event..." — was actually written to accommodate "SHOULD".)
Or maybe we use this opportunity to clarify the behavior of resumable streams. The current spec is not clear about what behavior is allowed for GET with
Last-Event-Id. The only thing disallowed seems to be delivery of messages that would have been delivered on a different stream. Does this mean that it is allowed to simply resume the stream without replaying any previously sent (or attempted to be sent) messages from the stream? Clarifying the language on this point could make resumable streams a much more reasonable burden on servers.
That seems like it might be a good change, though I'm not sure whether it would be acceptable as part of this PR. (Maybe so?)
I think we would want to say something like "when resuming a stream, the server MAY choose to not replay non-essential messages", but the spec would leave the qualification of "non-essential" to the server. So a server could choose to drop, e.g., progress notification messages if it deems those non-essential.
| and an empty `data` field in order to prime the client to reconnect (using | ||
| that event ID as `Last-Event-ID`). | ||
| - After the server has sent an SSE event with an event ID to the client, the | ||
| server **MAY** close the _connection_ (without terminating the _SSE stream_). |
There was a problem hiding this comment.
I think that closing the connection implies that the SSE stream is terminated. At least this is what ChatGPT tells me:
An HTTP server signals the termination of an SSE stream by closing the underlying HTTP connection — either by ending the response body (EOF) or by sending a connection close (FIN) at the TCP layer.
https://chatgpt.com/share/6910a7f6-7180-8013-9fdb-c675630f5db1
I think there is also potential confusion with the term "connection" since there are "connections" at the application layer (defined in the Lifecycle section of the spec) and "connections" at the transport layer.
Maybe use this wording instead:
| server **MAY** close the _connection_ (without terminating the _SSE stream_). | |
| server **MAY** close the stream before the JSON-RPC _response_ has been sent, |
There was a problem hiding this comment.
I think that closing the connection implies that the SSE stream is terminated.
I don't think that is correct. According to the SSE spec, the way for a server to terminate an SSE stream is to respond with a non-200 status code (typically a 204 if the stream terminates normally).
In order for the client to receive that status code, however, the server must first disconnect, which is a separate act. The client will then attempt to reconnect because it does not assume the stream was terminated. Only after the client receives the status code, will it exit the SSE processing loop.
(An alternative to all of the above is to send a custom "done" event that the client handles by exiting the SSE processing loop. But that would be a more invasive change, so this SEP does not take that approach.)
Maybe use this wording instead:
What I want to communicate here is that the server closes the network connection, but the conceptual stream is not terminated and can be resumed.
| that event ID as `Last-Event-ID`). | ||
| - After the server has sent an SSE event with an event ID to the client, the | ||
| server **MAY** close the _connection_ (without terminating the _SSE stream_). | ||
| - If the server closes the _connection_ prior to terminating the _SSE stream_, |
There was a problem hiding this comment.
Same suggestion here
| - If the server closes the _connection_ prior to terminating the _SSE stream_, | |
| - If the server closes the stream before the JSON-RPC _response_ has been sent |
| _request_. | ||
| - The server **SHOULD NOT** close the SSE stream before sending the JSON-RPC _response_ | ||
| for the received JSON-RPC _request_, unless the [session](#session-management) | ||
| - The server **MAY** terminate the SSE stream if the [session](#session-management) |
There was a problem hiding this comment.
Why the change from "close" to "terminate" here and below?
ChatGPT does seem to think that there is a subtle difference between "close" and "terminate".
https://chatgpt.com/share/6910ab14-92e8-8013-aac7-c68b3fbbeb1d
But I'm not clear that's the distinction we want to make here. In particular, I think we still want the server to "close", as opposed to "terminate", the SSE stream after sending the JSON RPC response.
There was a problem hiding this comment.
Why the change from "close" to "terminate" here and below?
But I'm not clear that's the distinction we want to make here. In particular, I think we still want the server to "close", as opposed to "terminate", the SSE stream after sending the JSON RPC response.
I'm trying to use consistent terminology to distinguish between closing the network connection and terminating the SSE stream.
The difference between the two is that the SSE stream can be resumed if the network connection is closed, but cannot be resumed if the SSE stream is terminated.
jonathanhefner
force-pushed
the
sep-1699
branch
from
2202f41 to
635f780
Compare
|
@jonathanhefner @mikekistler @pja-ant 👋 also just as a reminder, we're aiming to ensure that all SEPs that are on-deck for the |
|
This LGTM, but @jonathanhefner do you think we should just add a bit of non-standardese language around this to indicate the intent here: i.e. that if you don't want to hold open long-lived connections then they can use this affordance to disconnect and make the client retry periodically. I find that, while not binding in any way, these kind of statements can help uncover the intent in case we miss anything in the spec, and helps people infer any other specifics. |
pja-ant
left a comment
pja-ant
left a comment
There was a problem hiding this comment.
Oh, we need a Changelog entry too :)
This implements the specification changes for SEP-1699: * When a server starts an SSE stream, it MUST immediately send an SSE event consisting of an [`id`][SSE `id`] and an empty [`data`][SSE `data`] string in order to prime the client to reconnect with that event ID as the `Last-Event-ID`. Note that the SSE standard explicitly [permits setting `data` to an empty string][SSE empty `data`], and says that the appropriate client-side handling is to record the `id` for `Last-Event-ID` but otherwise ignore the event (i.e., not call the event handler callback). * At any point after the server has sent an event ID to the client, the server MAY disconnect at will. * If a server disconnects, the client will interpret the disconnection the same as a network failure, and will attempt to reconnect. In order to prevent clients from reconnecting / polling excessively, the server SHOULD send an SSE event with a [`retry`][SSE `retry`] field indicating how long the client should wait before reconnecting. Clients MUST respect the `retry` field. [SSE `id`]: https://html.spec.whatwg.org/multipage/server-sent-events.html#:~:text=field%20name%20is%20%22id%22 [SSE `data`]: https://html.spec.whatwg.org/multipage/server-sent-events.html#:~:text=field%20name%20is%20%22data%22 [SSE empty `data`]: https://html.spec.whatwg.org/multipage/server-sent-events.html#:~:text=data%20buffer%20is%20an%20empty%20string [SSE `retry`]: https://html.spec.whatwg.org/multipage/server-sent-events.html#:~:text=field%20name%20is%20%22retry%22
jonathanhefner
force-pushed
the
sep-1699
branch
from
635f780 to
528588d
Compare
That's a good point. I made the following change: ID and an empty `data` field in order to prime the client to reconnect
(using that event ID as `Last-Event-ID`).
- After the server has sent an SSE event with an event ID to the client, the
- server **MAY** close the _connection_ (without terminating the _SSE stream_).
+ server **MAY** close the _connection_ (without terminating the _SSE stream_)
+ at any time in order to avoid holding a long-lived connection. The client
+ **SHOULD** then "poll" the SSE stream by attempting to reconnect.
- - If the server closes the _connection_ prior to terminating the _SSE stream_,
+ - If the server does close the _connection_ prior to terminating the _SSE stream_,
it **SHOULD** send an SSE event with a standard `retry` field before
closing the connection. The client **MUST** respect the `retry` field,
waiting the given number of milliseconds before attempting to reconnect.
Let me know if you think we should elaborate more.
Done! |
5 participants
This implements the specification changes for SEP-1699:
When a server starts an SSE stream, it MUST immediately send an SSE event consisting of an
idand an emptydatastring in order to prime the client to reconnect with that event ID as theLast-Event-ID.Note that the SSE standard explicitly permits setting
datato an empty string, and says that the appropriate client-side handling is to record theidforLast-Event-IDbut otherwise ignore the event (i.e., not call the event handler callback).At any point after the server has sent an event ID to the client, the server MAY disconnect at will.
If a server disconnects, the client will interpret the disconnection the same as a network failure, and will attempt to reconnect. In order to prevent clients from reconnecting / polling excessively, the server SHOULD send an SSE event with a
retryfield indicating how long the client should wait before reconnecting. Clients MUST respect theretryfield.