Pass on supported gun opts in connect()

[uwiger]

This change allows users to pass on options supported by gun in a connect() call.

For example, this means that you can use alternative TLS port 2197 without gun guessing that you want TCP transport (add transport => tls), and you can set a longer keepalive than the default 5 seconds.

[ferigis]

Hi Ulf,

Thanks a lot for your contribution, could you please fix the Travis complains? after that I will merge it for sure!

thanks

[elbrujohalcon]

Just in case, @uwiger … the error that travis reports is:

The code in the following (LINE, COL) locations has the same structure: (237, 3), (251, 3).

It's an Elivs error that, unless you can abstract that code into something else that makes sense (probably not), you can "fix" by increasing min_complexity for the DRY rule in the elvis.config file.

[uwiger]

@elbrujohalcon feels like cheating, but you're right - I couldn't easily come up with a refactoring that wouldn't be less readable than the existing code. :)

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.99%. Comparing base (f3aba9e) to head (6724edc).
⚠️ Report is 52 commits behind head on master.

Files with missing lines	Patch %	Lines
src/apns_connection.erl	66.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
- Coverage   87.55%   86.99%   -0.57%     
==========================================
  Files           7        7              
  Lines         217      223       +6     
==========================================
+ Hits          190      194       +4     
- Misses         27       29       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[uwiger]

BTW, I noticed issue #205, which has been around for a while. Since gun defaults to sending keepalives every 5 seconds, I assume that the problem should be resolved. OTOH, a 5-second interval seems a bit eager for an APNs idle ping, right?

[uwiger]

As I understand it, the code coverage is reduced due to the changes related to the proxy setup. There don't seem to be any proxy-related test cases in the connection_SUITE.

[uwiger]

Since dialyzer run on the test code complained, I moved the gun options under a separate options key.

[elbrujohalcon]

Looks good to me.

[elbrujohalcon]

I guess we can safely ignore codecov… right, @ferigis?

[ferigis]

sure, we will fix the coverage later

[essen]

How did you get to a keepalive value of 10000? Trial and error? I'm looking into increasing the default to something more reasonable.

[uwiger]

How did you get to a keepalive value of 10000? Trial and error? I'm looking into increasing the default to something more reasonable.

I only used 10 seconds in the test suite. I assumed that we might want to set the value to something much higher, but it's surprisingly hard to find some recommendation on the subject.

I found this article, which admittedly talks about a device being connected to APNS, but it makes sense that Apple shouldn't force mobile, battery-powered devices to ping at intervals of a few seconds.

When device is charging or enough battery, keep-alive is sent every 15-30 minutes.

there is of course also this...:

HTTP/2 ping is a terrible idea. The best we can hope for is that nobody uses it. That boat has sailed already though, as Apple is encouraging the PING frame use for checking the state of TCP connections to their APNS service.

[essen]

Thanks.

I think the biggest improvement would be to make Gun NOT send the PING if there's been traffic sent or received in that time period. Then maybe set it to 1 minute by default, since there is no real power constraint. So Gun would by default send PING 1 minute after any data was sent or received.

To be honest there probably aren't too many people with mostly idle connections, so it might not make sense for it to be enabled by default. Not having it enabled would help avoid certain issues like too_many_pings. Having written all this I think this makes the most sense.

[paulo-ferraz-oliveira]

Is this not merged solely due to Travis CI complaints? Or is there something else missing? I read the whole thread and this seems good to go.

@ferigis