Skip to content

Handle keep-alive behavior to close the connection #201

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

Merged
merged 9 commits into from
Jun 5, 2021

Conversation

sneko
Copy link
Contributor

@sneko sneko commented Apr 7, 2021

Hi @leszekhanusz ,

Here the PR for the keep-alive behavior #200, tested with real servers sending ka messages.

I read the CONTRIBUTING.md but struggled with the make check, I installed them but it changes all my files (including yours) like if parameters were not taken in account. Moreover isort tells me some lines are >88chars but no other tools automatically adjust them (I thought it would?). I guess I missed something... I did the venv creation + make dev-setup.

Tip for others: in my app code I forgot to await ws_transport.wait_close() making the reconnection after a keep-alive failure failing because some stuff were not clean in the meantime (asyncio giving the priority to other tasks including mines trying a reconnect).

Tell me if I need to adjust something :)

@sneko
Copy link
Contributor Author

sneko commented Apr 7, 2021

@leszekhanusz I fixed the virtualenv so it's not longer modifying lot of things.

Note: from https://github.com/graphql-python/gql/blame/master/CONTRIBUTING.md#L34
it should be instead writing as inside your Makefile with quotes: python pip install -e ".[dev]"
by the way, in the .md it's about dev and in the Makefile about [test], is it intended?

Now I'm still getting the line length errors... I'm wondering which tool should be managing this.

EDIT: I pushed a manual adjustment for lines length... sounds hacky and looks a bit weird. I'm curious what you use to make this automatic? The make check did nothing for that for me. (on my Python projects I'm using autopep8)

@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2021

Codecov Report

Merging #201 (78919a6) into master (4528977) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #201   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines          985      1008   +23     
=========================================
+ Hits           985      1008   +23     
Impacted Files Coverage Δ
gql/transport/websockets.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4528977...78919a6. Read the comment docs.

modifications:
* clean_close = False
* keep_alive_timeout is now Optional[int] default None
* calling self._fail directly from the _check_ws_liveness coro
* no need to cancel the _receive_data_loop coro, it will stop itself
  once the websocket will close
@leszekhanusz
Copy link
Collaborator

@sneko Could you please check if the refactor is working correctly for you ?

@leszekhanusz leszekhanusz linked an issue May 24, 2021 that may be closed by this pull request
@leszekhanusz leszekhanusz requested a review from KingDarBoja May 24, 2021 17:16
@leszekhanusz leszekhanusz merged commit 35203e8 into graphql-python:master Jun 5, 2021
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.

Any plan for handling "ka" (keep-alive) and ping-pong checks?
3 participants