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 ping functionality #51

Closed
wants to merge 1 commit into from
Closed

Conversation

gngeorgiev
Copy link

@gngeorgiev gngeorgiev commented Nov 23, 2016

This was not supposed to get here so soon, submitted it by mistake, but I am gonna leave it for discussion.

return c.pinging
}

func (c *Client) GetPingInterval() time.Duration {

Choose a reason for hiding this comment

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

exported method Client.GetPingInterval should have comment or be unexported

@@ -88,6 +152,14 @@ func (c *Client) Production() *Client {
return c
}

func (c *Client) IsPinging() bool {

Choose a reason for hiding this comment

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

exported method Client.IsPinging should have comment or be unexported

}()
}

func (c *Client) DisablePinging() {

Choose a reason for hiding this comment

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

exported method Client.DisablePinging should have comment or be unexported

return client
}

func (c *Client) EnablePinging(pingInterval time.Duration, pingError chan error) {

Choose a reason for hiding this comment

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

exported method Client.EnablePinging should have comment or be unexported

@coveralls
Copy link

coveralls commented Nov 23, 2016

Coverage Status

Coverage decreased (-8.5%) to 81.149% when pulling a6aec72 on gngeorgiev:ping into 37557e7 on sideshow:master.

@sideshow
Copy link
Owner

sideshow commented Jan 3, 2017

@gngeorgiev I copied this comment from #45 as the same applies.

  • The problem is that the go HttpClient doesn't just use one connection, but rather has an underlying connection pool. The ConnPool manages a pool of HTTP/2 client connections - You are only holding on to the last connection that was made to the server, and sending ping frames to that. So if you have 10 say connections because you have been smashing APNs and are only pinging the last connection its not going to fully solve the problem. This may be working in your scenario because you aren't sending enough to cause the client connection pool to open more than one connection.

  • For the same reason above this code would make more sense at the transport level, or in a custom client connection pool level rather than in the Client level. As you mentioned it would be ale to also help with issue add ping pong functionality #45.

Thanks

@gngeorgiev
Copy link
Author

@sideshow Thank you for the reply.

Alright, so the HttpClient has a pool of connections, fair enough. Will the Dial callback be called for each new connection? If yes, this can be easily fixed. If not, what can we do? Currently, it is working in our scenario and we have send thousands of push notifications, sometimes with bursts of 50k but I am always looking to improve things.

As to where the feature is implemented - believe me, I spend a considerable amount of time trying to implement this feature on a lower level. I don't remember the specifics since a while passed since I worked on this, but simply not enough stuff were exposed so you can make it civilized. That's why in the end I had to implement it in the client. Sure, we can abstract this in some data structure outside the client and it will use it, this could make things better, I guess, but not perfect in any case.

@sideshow
Copy link
Owner

Yes the Dial callback will be called for each new connection. You could certainly hold on to these connections, but you also dont know when they are closed. I'm closing this issue, but have logged an issue/enhancement ticket around creating a better connection pool which could have this functionality #88 thanks

@sideshow sideshow closed this Aug 23, 2017
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.

4 participants