Keep EventSource alive until all subscriptions are closed#545
Conversation
| //! | ||
| //! // The EventSource can be dropped while subscriptions are still active. | ||
| //! // The underlying connection stays alive as long as any subscription exists. | ||
| //! drop(es); |
There was a problem hiding this comment.
I really don't like that this is possible at compile time but I don't see another way to fix the bigger issue of silent EventSource closures without lifetime pains
Madoshakalaka
left a comment
There was a problem hiding this comment.
Solid changes. However, dropping an EventSource no longer closes the connection or notifies subscribers. Users who relied on drop(es) (or letting es go out of scope) to signal teardown to subscribers now need to call es.close() instead.
This is a behavioral breaking change even though the API surface is nearly identical. It should be called out in a CHANGELOG entry, something like:
Breaking:
EventSourceno longer closes the underlying connection on drop. The connection now stays alive as long as anyEventSourceSubscriptionexists. CallEventSource::close()to shut down immediately.
Done. The root version is not updated in the CHANGELOG as that is done by another PR |
Update
EventSourceSubscriptionto hold anRc<EventSource>to prevent premature closure of the EventSource.While I would've preferred to hold a
&EventSourceinEventSourceSubscription, that would forceEventSourceSubscriptionto not be'static, which is needed forspawn_locals and borrows would make lifetimes a nightmare to deal withfixes #495