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[bot]]

Codecov Report

Merging #230 into master will decrease coverage by 0.56%. The diff coverage is 66.66%.

@@            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     

Impacted Files	Coverage Δ
src/apns_connection.erl	79.10% <66.66%> (-0.59%)	:arrow_down:

---

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 f3aba9e...6724edc. Read the comment docs.

[uwiger]

BTW, I noticed issue inaka/apns4erl#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]

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

[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