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

Implement connection checks using PING frame. Task #62 #94

Closed
wants to merge 2 commits into from

Conversation

a00920
Copy link

@a00920 a00920 commented Nov 12, 2019

No description provided.

@vmagamedov
Copy link
Owner

Awesome, thanks! I don't have any complains about your implementation, but would you mind to write also a test?

@a00920
Copy link
Author

a00920 commented Nov 18, 2019

@vmagamedov

Tests added.

@vmagamedov
Copy link
Owner

Thanks! But now I need to discover a new way to configure client/server, instead of ping_delay and ping_timeout arguments, and do this before merging this PR into master.

As you can see here, gRPC Core already has plenty of configuration options. I hope that grpclib will also have similar configuration options in the future.

When I come up with a design of these options I would merge this PR.

@vmagamedov
Copy link
Owner

Turns out there are lots of things to consider:
https://github.com/grpc/proposal/blob/master/A8-client-side-keepalive.md
https://github.com/grpc/grpc/blob/master/doc/keepalive.md

Current implementation will simply cause a GOAWAY frame (disconnect) from the grpcio-based server, because it sends PING frames so frequently.

@vmagamedov
Copy link
Owner

Merged my WIP channelz branch into master to make it possible to implement keepalive feature properly. Now we know when last DATA frame was sent into connection. Also merged my config branch into master, now we have grpclib.config.Configuration class to configure grpclib.

@a00920
Copy link
Author

a00920 commented Dec 4, 2019

started from the beginning in another branch
#96

@a00920 a00920 closed this Dec 4, 2019
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