ostinelli / apnotic

A Ruby APNs HTTP/2 gem able to provide instant feedback.
MIT License
480 stars 96 forks source link

Cannot rescue from OpenSSL::SSL::SSLError #118

Open collimarco opened 2 years ago

collimarco commented 2 years ago

We are using this gem with sync pushes: https://github.com/ostinelli/apnotic#sync-pushes

This is the relevant code:

def deliver_to_apns
  connection = Apnotic::Connection.new(cert_path: StringIO.new(apns_cert))
  notification = Apnotic::Notification.new(device_token)
  notification.alert = { title: title, body: body }
  notification.url_args = ['foo', 'bar']
  response = connection.push(notification, timeout: 30)
  connection.close
  handle_apns_response response
rescue OpenSSL::SSL::SSLError
  Rails.logger.error "APNs certificate expired"
  return :unsuccessful
end

The problem is that OpenSSL::SSL::SSLError is not catched by rescue, as you would expect.

Basically the exception is raised, but rescue has not effect and the rescue code does not get executed.

Is that because we don't set connection.on(:error)? Is there any way to raise all the exceptions normally so that the rescue in the above code can work? Or any workaround?

Thanks

collimarco commented 2 years ago

Another related question...

If I use the sync push, and I add the "on error" callback:

connection = Apnotic::Connection.new(cert_path: StringIO.new(apns_cert))
connection.on(:error) { |exception| puts "Exception has been raised: #{exception}" }

What happens to the main code when there is an exception in the background thread?

Does connection.push raises an exception? Does it return nil? Or something else happens?

collimarco commented 2 years ago

What happens to the main code when there is an exception in the background thread? Does connection.push raises an exception? Does it return nil?

Ok, it seems that the call connection.push waits 30 seconds (timeout) and then returns nil... This is an unexpected behavior.

If the APNs certificate is expired (or a similar error occurs), why wait 30 seconds in the connection.push call?

mcfoton commented 2 years ago

We're seeing the same behavior, connection.push keeps holding the connection for 60 seconds (NetHttp2::Client's default) till it times out.

In our case we are using a connection pool, so the issue has an even wider scope:

  1. Async job is invoked 5 times to send 5 push notifications
  2. For some reason Apple closed all 5 connection we had in the pool
  3. 5 invocations of connection_pool.with grab all 5 connections. Immediately Connection reset by peer is raised and caught in the on(:error) definition, but the connections themselves are not checked in and are held for 60 seconds

Thus, any new invocation of the async job will raise ConnectionPool::Timeout: waited for 5 seconds because all connections are in use.

Probably the connection should be checked in/shutdown after the error is returned? How can this be achieved?


@collimarco the nil on timeout is indeed unexpected, but it's in the docs https://github.com/ostinelli/apnotic#blocking-calls

mcfoton commented 2 years ago

update: Apologies for hijacking, but my case seems relevant, even though we're running with connection pool. Also this seems related to connection timeouts

After some digging, it seems that the issue might be related to net-http2's logic here

# /lib/net-http2/client.rb

def call(method, path, options={})
  request = prepare_request(method, path, options)
  ensure_open
  new_stream.call_with request
end

Right now even if ensure_open failed and ended up running the code inrescue EOFError, emitting the on(:error) for the Apnotic connection, the new_stream.call_with request will still be called and will wait for the whole timeout duration 🤔

benubois commented 2 years ago

Hi @mcfoton,

If you want to make a pull request to address this it would be greatly appreciated.

Apnotic is mostly community supported at this point so that would be the main way for this to get fixed.

Thanks!

ostinelli commented 2 years ago

Thank you @benubois.

mcfoton commented 2 years ago

@benubois @ostinelli I'll see if I can do something about this, though the topic of threads is pretty new to me. Hopefully what I described above is at least the right direction 😌

And just for the record: another unhandled exception that we are seeing is Errno::ECONNRESET thrown by ruby-2.7.3/lib/ruby/2.7.0/openssl/buffering.rb:322 and is also initially called by net-http2's call method with new_stream.call_with request.