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 #96

Closed
wants to merge 8 commits into from

Conversation

a00920
Copy link

@a00920 a00920 commented Dec 4, 2019

No description provided.

self._ping_handle = None
return
current_time = time.monotonic()
time_to_next_ping = self._config._keepalive_time - (
Copy link
Owner

Choose a reason for hiding this comment

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

According to https://github.com/grpc/proposal/blob/master/A8-client-side-keepalive.md

Restricts clients to avoid configuring their keepalive below ten seconds

We have to limit somehow minimum time between two consequent pings

Copy link
Owner

@vmagamedov vmagamedov left a comment

Choose a reason for hiding this comment

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

There is one unresolved comment from the previous review.

Comment on lines +599 to +600
self.connection.last_headers_received = time.monotonic()
self.connection.data_process()
Copy link
Owner

Choose a reason for hiding this comment

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

I think that the same should be done in process_response_received and in process_trailers_received, they all are called after we receive HEADERS frame.

Copy link
Author

Choose a reason for hiding this comment

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

Now it seems to me that the received message and headers do not interest us.
This channel argument controls the maximum number of pings that can be sent when there is no other data (data frame or header frame) to be sent

Copy link
Owner

Choose a reason for hiding this comment

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

Agreed

Copy link
Author

Choose a reason for hiding this comment

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

done

vmagamedov added a commit that referenced this pull request Jan 23, 2020
@vmagamedov
Copy link
Owner

Squashed and merged PR into master with some fixes. 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.

2 participants