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

added timeout to create_websocket() #286

Merged
merged 6 commits into from
Jul 19, 2022

Conversation

jpfarias
Copy link
Contributor

Currently the create_websocket() call doesn't have any timeout which means it can wait forever for the websocket to connect.

I made it use the same request_timeout as the one used for pooling connection.

This is a pretty small patch, hope it won't be a problem to merge.

@miguelgrinberg
Copy link
Owner

Would it be possible to implement the same change on the async client? And adding a test that ensures that the request timeout is used when it is not set to the default? Thanks!

@jpfarias
Copy link
Contributor Author

Yeah, will do! Will update the PR when it is done.

@jpfarias
Copy link
Contributor Author

I believe this is it for the asyncio as well. Thanks!

@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2022

Codecov Report

Merging #286 (a6f4131) into main (cb2d7a6) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main      #286   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines         1934      1934           
  Branches       377       377           
=========================================
  Hits          1934      1934           
Impacted Files Coverage Δ
src/engineio/asyncio_client.py 100.00% <ø> (ø)
src/engineio/client.py 100.00% <ø> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@jpfarias
Copy link
Contributor Author

Sorry for the many commits, I am new to contributing and wasn't aware of all the checks. Hopefully everything is ok now. Thanks again!

@miguelgrinberg miguelgrinberg merged commit f6df30b into miguelgrinberg:main Jul 19, 2022
@miguelgrinberg
Copy link
Owner

Thanks!

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.

3 participants