Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#89] Add SSE Send Timeout #90

Merged
merged 7 commits into from
Jan 25, 2024
Merged

Conversation

blodow
Copy link
Contributor

@blodow blodow commented Jan 22, 2024

As described in #89, this adds a send_timeout parameter to the EventSourceResponse initializer. In the event of the send call inside the response's sending logic exceeding this duration, a SendTimeoutError is raised, aborting the SSE event generator and effectively canceling the HTTP connection.

* a client connection that is not read from can block the SSE server task.
@sysid
Copy link
Owner

sysid commented Jan 22, 2024

@blodow, thank you for this PR, much appreciated! Please give me some time to review it properly.

@sysid
Copy link
Owner

sysid commented Jan 23, 2024

@blodow , following feedback

  1. Your use-case is valid and your PR as well.
  2. make test was not working, which I fixed.

After upgrade dependecies:

  1. tests via make tox are breaking for python 12: anyio has got breaking changes: ExceptionGroups

At this point I need to stop due to time constraints. However, make tox needs to pass in order to get the PR merged.
Maybe you can have a look at it.

@blodow
Copy link
Contributor Author

blodow commented Jan 24, 2024

@sysid I think I managed to fix the remaining issues in a sort of minimally-invasive way. Let me know if you would like to see any other changes.

Copy link
Owner

@sysid sysid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@sysid sysid merged commit e3d46d7 into sysid:main Jan 25, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants