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

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

Open
collimarco opened this issue Mar 9, 2022 · 7 comments
Open

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

collimarco opened this issue Mar 9, 2022 · 7 comments

Comments

@collimarco
Copy link

collimarco commented Mar 9, 2022

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
Copy link
Author

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
Copy link
Author

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
Copy link

mcfoton commented May 1, 2022

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
Copy link

mcfoton commented May 1, 2022

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
Copy link
Collaborator

benubois commented May 1, 2022

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
Copy link
Owner

Thank you @benubois.

@mcfoton
Copy link

mcfoton commented May 9, 2022

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

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

No branches or pull requests

4 participants