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

Using Apnotic::ConnectionPool with Sidekiq is unsafe #69

Closed
krasnoukhov opened this issue Jul 14, 2018 · 5 comments
Closed

Using Apnotic::ConnectionPool with Sidekiq is unsafe #69

krasnoukhov opened this issue Jul 14, 2018 · 5 comments

Comments

@krasnoukhov
Copy link
Contributor

krasnoukhov commented Jul 14, 2018

From net-http2 documentation:

It is RECOMMENDED to set the :error callback: if none is defined, the underlying socket thread may raise an error in the main thread at unexpected execution times.

But the problem is that Apnotic::ConnectionPool does not allow calling on on actual Apnotic::Connection to pass in the error handler.

Here's example scenario that leads to lost jobs - we've been seeing this in production for quite some time now but couldn't point a finger on it, but thanks to #68 it's got easily reproducible:

  • There's a sidekiq worker set up to initialize pool as suggested in documentation with no error handler
  • Fresh sidekiq process is booted and it starts processing jobs, some of them call that worker
  • APNS connection fails with SocketError or similar
  • Sidekiq process crashes completely since that SocketError is raised on a main thread
  • Unprocessed jobs that were already picked up by that process are lost completely

Here is an example report of this exact behavior: sidekiq/sidekiq#3886

I think apnotic should definitely do a better job here to improve reliability and also stop suggesting unsafe usage in documentation. Here's what I would suggest:

  • Allow to pass through connection error handler into Apnotic::ConnectionPool
  • Make this handler required or at least change the documentation to have it explicitly provided

Currently we work this around by creating a pool manually:

class Worker
  POOL = ConnectionPool.new(size: 5) do
    connection = Apnotic::Connection.new(...)

    connection.on(:error) do |err|
      Bugsnag.notify(ConnectionError.new(err.inspect))
    end

    connection
  end
end

Please let me know if there are any thoughts. Thanks!

@ostinelli
Copy link
Owner

Thank you for reporting this! Care to issue a PR for both of your points?

@krasnoukhov
Copy link
Contributor Author

I could do that, do you have any preference on the API for this?

@ostinelli
Copy link
Owner

ostinelli commented Jul 16, 2018

One option is to allow users to set a connection after the initialization on the connection pool:

APNOTIC_POOL = Apnotic::ConnectionPool.new({
  cert_path: Rails.root.join("config", "certs", "apns_certificate.pem"),
  cert_pass: "mypass"
}, size: 5).do |connection|
  connection.on(:error) { |exception| puts "Exception has been raised: #{exception}" }
end

Still thinking whether we should set this by default or not on:
https://github.com/ostinelli/apnotic/blob/master/lib/apnotic/connection_pool.rb

What do you think?

@krasnoukhov
Copy link
Contributor Author

krasnoukhov commented Jul 17, 2018

My feeling is that it should be required so user can decide how to deal with those errors. I'm sure that for the most of the production systems, some kind of exception tracking tool is used (Rollbar/Bugsnag/Honeybadger/etc) so it could be utilized accordingly. I created a PR, let me know what you think

@ostinelli
Copy link
Owner

Merged, thank you for your help.

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

2 participants