Issue sending development/sandbox notifications

[axelson]

Within the last 24 hours we've been unable to send push notifications in development/sandbox environment.

It looks like one issue may be that the development url used by apnotic doesn't match apple's docs.

https://developer.apple.com/documentation/usernotifications/setting_up_a_remote_notification_server/sending_notification_requests_to_apns#overview has:

* Development server: api.sandbox.push.apple.com:443
* Production server: api.push.apple.com:443

While the current master branch of apnotic sets APPLE_DEVELOPMENT_SERVER_URL to "https://api.development.push.apple.com:443"

apnotic/lib/apnotic/connection.rb

Line 6 in f11ff56

APPLE_DEVELOPMENT_SERVER_URL = "https://api.development.push.apple.com:443"

However, setting the url to https://api.sandbox.push.apple.com:443 does not fix the issue.

We observe the issue with a stack trace like this:

Exception: SocketError: Socket was remotely closed
--
0: /Users/jason/.rbenv/versions/2.4.4/lib/ruby/gems/2.4.0/gems/net-http2-0.16.0/lib/net-http2/client.rb:130:in `callback_or_raise'
1: /Users/jason/.rbenv/versions/2.4.4/lib/ruby/gems/2.4.0/gems/net-http2-0.16.0/lib/net-http2/client.rb:115:in `rescue in block (2 levels) in ensure_open'

[aanosh]

I have the same issue

[ostinelli]

Apparently Apple has started enforcing some additional requirements on HTTP2. @byronformwalt has done some digging and written to both me an @igrigorik:

In the specification section 8.1.2.1 (https://http2.github.io/http2-spec/#rfc.section.8.1.2.1), it says that "All pseudo-header fields MUST appear in the header block before regular header fields. Any request or response that contains a pseudo-header field that appears in a header block after a regular header field MUST be treated as malformed (Section 8.1.2.6).”

Digging in a little further, it appears that this issue may actually be a problem with the http-2 gem authored by Ilya (cc’d).

Ilya,

I have traced through the http-2 gem source beginning at line 155 of http-2-0.9.0/lib/http/2/stream.rb and it seems that the gem could benefit from an intelligent reordering of the headers prior to encoding them on line 449 of http-2-0.9.0/lib/http/2/compressor.rb. In accordance with https://http2.github.io/http2-spec/ section 8.1.2.1, such that pseudo-header fields are encoded and subsequently transmitted first. It appears that Apple’s development APN service just started rejecting headers last night based on this requirement.

I plan to attempt to work around the issue by pre-injecting the required pseudo headers into my headers hash prior to calling the net-http2 client, the ordering of which will hopefully be respected all the way through to the http-2 gem’s header command compressor.

Let's see if Ilya has some time to fix this at https://github.com/igrigorik/http-2 level, or eventually I will do so at https://github.com/ostinelli/net-http2 level.

[byronformwalt]

While, I am not using Apnotic directly, here is how I worked around the issue:

headers.merge!(
{
  ":scheme" => @uri.scheme,
  ":method" => "POST",
  ":path" => path,
  ":authority" => "#{@uri.host}:#{@uri.port}"
})
headers = headers.sort.to_h

This will work for as long as all of the keys in your header are string comparable.

[igrigorik]

@byronformwalt thanks for reporting this! Would you be up for creating a PR+spec test to address this? Happy to help with code reviews, etc.

[ioquatix]

I don't think this belongs in http-2.

async-http avoids this issue by explicitly specify these headers first.

https://github.com/socketry/async-http/blob/f3f984999835ae8b195cd6b7aaafae2ed8968a12/lib/async/http/protocol/http2.rb#L334-L339

Assuming that the argument passed to headers will respond_to?(:sort) would break my code. It's also inefficient.

headers.merge!(
{
  ":scheme" => @uri.scheme,
  ":method" => "POST",
  ":path" => path,
  ":authority" => "#{@uri.host}:#{@uri.port}"
})
headers = headers.sort.to_h

Is completely avoidable and inefficient. The only case I can see for it would be when you expect users to provide headers starting with : and even then you could expect them to pass an array in that case.

As another example why using a hash isn't great - some headers can be set multiple times, and obviously you can't do this with a hash.

[byronformwalt]

@ioquatix : That was my fix prior to calling the net-http2 client. It is not intended to be used in either net-http2 nor http2. I apologize for any misunderstanding.

@igrigorik : I have submitted a pull request with recommended changes to the http2 gem, and am looking forward to receiving your feedback. Travis CI is choking on the PR, due to an environment variable issue, but all tests pass on my local machine.

[ioquatix]

I also apologise if I've misunderstood something.

I will take a look at the PR.

[aalsanad]

Hey guys @ostinelli @igrigorik @byronformwalt
Thanks for the great work you are doing!
I just want to let you know that we’re impatiently waiting for a solution for this!

[ioquatix]

I took a look at net-http2 and the way it builds headers could be improved - probably better to use an array rather than a hash and use concatenation rather than merge: https://github.com/ostinelli/net-http2/blob/ca959d914e3e39686b525ca5865a9d08b253a1d1/lib/net-http2/request.rb#L25-L43

I would suggest again reviewing how https://github.com/socketry/async-http/blob/f3f984999835ae8b195cd6b7aaafae2ed8968a12/lib/async/http/protocol/http2.rb#L334-L339 works as it's the simplest solution to get something working ASAP and is also the most efficient.

I hope we can make some changes to http-2 to improve the situation, but at the very least it also probably needs attention in net-http2.

[ioquatix]

Here is the current discussion in http-2: igrigorik/http-2#132

[ostinelli]

@ioquatix you even thumb up your own posts now? :)

The implementation of headers in net-http2 will be revised, but this has nothing to do with the discussion on where they should be sorted.

As a side note: while I appreciate your contribution and passion, I'd ask you to stop the constant reference/advertisement of a gem that is completely outside the scope of this discussion. Thank you.

[ostinelli]

I hope we can make some changes to http-2 to improve the situation, but at the very least it also probably needs attention in net-http2.

IMO no it shouldn't. This is a HTTP/2 protocol issue and hence it should be addressed in the gem that implements the HTTP/2 protocol (http-2). However, if the necessary additions to http-2 will not be made for some reason, I will be forced indeed to address the issue in net-http2. i will wait a few days to see where you go with this and adapt accordingly.

[ioquatix]

@ioquatix you even thumb up your own posts now? :)

Whoops, I meant to thumb up @aalsanad :)

The implementation of headers in net-http2 will be revised, but this has nothing to do with the discussion on where they should be sorted.

Fair enough. I see this as a cross layer concern: if it's implemented in http-2 it won't need to be implemented in net-http2.

As a side note: while I appreciate your contribution and passion, I'd ask you to stop the constant reference/advertisement of a gem that is completely outside the scope of this discussion. Thank you.

I think it's reasonable to reference a different implementation showing how to handle headers, but as you wish. I won't get involved any further.

[ostinelli]

Whoops, I meant to thumb up @aalsanad :)

Lol :)

Got your points. Sorry for quick messages, it's late and it has been a long day. I'll check what you and @igrigorik decide in the fore-coming days and react accordingly.

[ioquatix]

Whoops, I meant to thumb up @aalsanad :)

It's equally been a long week for me so far - two kids sick, I got sick, we are all human. Let's try to keep things easy here, as you say we are all passionate and trying to find the right solution.

Got your points. Sorry for quick messages, it's late and it has been a long day. I'll check what you and @igrigorik decide in the fore-coming days and react accordingly.

Take it easy and get some rest :)

[ostinelli]

A new version of net-http2 has been released which should solve this issue.

I have to say that I am embittered by the way that this issue is being handled by @ioquatix, one of http-2 collaborators, whose concerns seem to dissociate from what the gem owner (@igrigorik), the person who found the solution to this issue (@byronformwalt) and myself agree on, which is to reorder the headers at http-2 level.

I'm confident they will untangle the issue in the best possible way, however in the meantime I am disengaging from further discussion on the matter. @axelson and @tommy-gansta can you please confirm updating net-http2 to 0.18.0 solves this for you so I can close the issue? I will remove any comments which do not simply state whether this is working or not, in the same way igrigorik/http-2#132 was locked since "too heated". Thank you.

[khadijakarkhanawala]

@ostinelli Thank you for fixing this issue. New update has solved it for our project. :)

[ostinelli]

Thank you for the feeeback, closing.

[axelson]

Works for me too. Thanks!

[augustosamame]

Just wanted you to know that this issue started biting us in production since this morning. It seems Apple implemented the change in production as well. Upgrading to latest master (and underlying http libraries) fixed the issue for us. I will open up an issue specific to the error message we where getting so that others may implement the fix.

[axelson]

Thanks for letting us know @augustosamame, although it would be nice if Apple would notify us of these things. As far as I can tell there was no warning.

[byronformwalt]

@axelson: Yes, but I can definitely see why @apple would not view it to be in their best interest to advertise that their server was out of compliance. Pushing compliance changes to their sandbox several weeks before rolling them out into production is a clever way to minimize the collateral damage without directly copping to the issue. I’m grateful we caught it in time, but I am disappointed by the apparent lack of transparency from responsible team within @apple.

If anyone is aware of a technical note published by @apple that detailed the change in advance, I invite you to post the details here, so that others in the iOS development community can subscribe to critical updates before they impact production servers/apps.

@apple: I still think you're awesome! 🥇