sysid / sse-starlette

BSD 3-Clause "New" or "Revised" License
505 stars 36 forks source link

Client disconnect revisited, role of competing detection mechanisms #28

Closed boamaod closed 2 years ago

boamaod commented 2 years ago

This is rather a request for clarification and not a bug report, because my code inspired from example works fine and issue has been thoroughly discussed already at https://github.com/sysid/sse-starlette/issues/7, but after commit https://github.com/sysid/sse-starlette/commit/0f69f5618133485151a09ffbc6325b33b3b9c02f the example seems to suggest two different ways of detecting client disconnect. In my implementation only CancelledError at line 67 is reached and req.is_disconnected() at line 59 appears unreachable in case of actual disconnect.

https://github.com/sysid/sse-starlette/blob/04526968bbe781320835d71f9a2a1b77fe21365a/examples/example.py#L56-L70

What is the role of those different detections of client disconnect and are they both needed? Since the issue is also featured in README.md this might be maybe worth an extra clarification.

paxcodes commented 2 years ago

 In my implementation only CancelledError at line 67 https://github.com/sysid/sse-starlette/blob/04526968bbe781320835d71f9a2a1b77fe21365a/examples/example.py#L67 is reached and req.is http://req.is/_disconnected() at line 59 https://github.com/sysid/sse-starlette/blob/04526968bbe781320835d71f9a2a1b77fe21365a/examples/example.py#L67 appears unreachable in case of actual disconnect.

From what I understand, this behaviour is expected since client disconnections are now handled by sse-starlette as documented in the issue and the commit you referenced.

As I see it, the examples need to be updated to no longer use req.is_disconnected() and rather use CancelledError (I think it's more pythonic, too!)


Margret "Pax" Williams | GitHub https://github.com/paxcodes | Portfolio https://margret.pw | LinkedIn https://www.linkedin.com/in/margret-pax-williams/

21 Jan 2022, 20:50 by @.***:

This is rather a request for clarification and not a bug report, because my code inspired from > example https://github.com/sysid/sse-starlette/blob/master/examples/example.py> works fine and issue has been thoroughly discussed already at > #7 https://github.com/sysid/sse-starlette/issues/7> , but after commit > 0f69f56> the example seems to suggest two different ways of detecting client disconnect. In my implementation only > CancelledError> at > line 67 https://github.com/sysid/sse-starlette/blob/04526968bbe781320835d71f9a2a1b77fe21365a/examples/example.py#L67> is reached and > req.is_disconnected()> at > line 59 https://github.com/sysid/sse-starlette/blob/04526968bbe781320835d71f9a2a1b77fe21365a/examples/example.py#L67> appears unreachable in case of actual disconnect.

https://github.com/sysid/sse-starlette/blob/04526968bbe781320835d71f9a2a1b77fe21365a/examples/example.py#L56-L70

What is the role of those different detections of client disconnect and are they both needed? Since the issue is also featured in > README.md https://github.com/sysid/sse-starlette/blob/04526968bbe781320835d71f9a2a1b77fe21365a/README.md> this might be maybe worth an extra clarification.

— Reply to this email directly, > view it on GitHub https://github.com/sysid/sse-starlette/issues/28> , or > unsubscribe https://github.com/notifications/unsubscribe-auth/ADIDWNTLY725GCEVE6XSYA3UXIZPZANCNFSM5MRHDQSA> . You are receiving this because you are subscribed to this thread.> Message ID: > <sysid/sse-starlette/issues/28> @> github> .> com>

sysid commented 2 years ago

That is correct @paxcodes. Thanks both of you for making this transparent. I will update the example.

sysid commented 2 years ago

Changes are merged. #30