alloy / lowdown

A Ruby client for the HTTP/2 version of the Apple Push Notification Service.
MIT License
120 stars 15 forks source link

Long running singleton usage #7

Closed dereklucas closed 8 years ago

dereklucas commented 8 years ago

Not really an issue, but here goes.

In my app I generate and store a client per worker that is essentially client = Lowdown::Client.production(production_env, certificate: cert, pool_size: 3) and then the worker essentially runs:

    client.connect do |group|
      tokens.each do |t|
        notif = Lowdown::Notification.new( etc )

        group.send_notification(notif) {}
      end
    end

But I believe that's disconnecting the client in the middle of the next group sending. I'm getting a few different errors, things like

ERROR -- : Lowdown::Connection::TimedOut: Initiating SSL socket timed-out. - retrying in 5 seconds (1/5)

ERROR -- : IOError: closed stream - retrying in 5 seconds (1/5)

ERROR -- : Actor crashed!
IOError: closed stream

Previously I had been connecting the client before storing it, which resulted in the CPU pegged at 100% constantly. My thought was to change from the block connect to something like client.connect unless client.connection.connected? with a disconnect after the group is run, which prompted me to ask if I could tell if a connection was being used.

I'm starting to think maybe I'm not actually following how this is working. :smile:

alloy commented 8 years ago

What’s your intended usage, to keep the connections open or only connect for short periods when needed?

alloy commented 8 years ago

Btw, I made a change to the examples/simple.rb script to simulate singleton client that connects and re-connects a couple of times, which appears to run fine. Let me know if and how your situation differs.

alloy commented 8 years ago

Also, I’m correct in understanding that at least 1 group of notifications gets sent successfully, right?

dereklucas commented 8 years ago

Ok, reviewing everything, and what I wrote previously was sort of a mashup of three separate things, some still in progress, so it's not necessarily accurate. I think it's safe to ignore the disconnect issues I was having.

Here's where things stand now: Running the first code block (Essentially your examples/simple.rb setup, where we store the client, and then run a connect block on the client) and the CPU issues seemed fine, but then we noticed it was leaking memory: screen_shot_2016-01-29_at_4 50 33_pm_480 (The dip is restarting Puma to free the memory)

So we had set our send method to return before it got to the connect block, thinking we'd deal with it today. That fixed the memory leak, but then the CPU started spiking again. I don't have a chart of the new spike, but here's the clear CPU spike when we were running things before. screen_shot_2016-01-29_at_9 59 56_am

alloy commented 8 years ago

It seems weird that you’d get a CPU spike when you’re not even connecting at all.

Can you try replicate your situation(s) in (an) example script(s)?

dereklucas commented 8 years ago

Yep, that's what I'm working on now.

alloy commented 8 years ago

Ace :+1:

alloy commented 8 years ago

I’ve pushed some changes to the examples so that they keep on running and took some memory measurements, both seem to be working fine:

simple.rb

simple

(The maxed out period is where I disconnected for a bit and it kept the notifications in the queue.)

long-running.rb

long-running

alloy commented 8 years ago

@dereklucas Btw, maybe you can also try the ‘delegate’ method instead of a block to exclude possible memory leaks due to block retain cycles https://github.com/alloy/lowdown#grouping-requests

alloy commented 8 years ago

@dereklucas Any update on this?

dereklucas commented 8 years ago

Hey sorry I got pulled onto something else.

​Our issues were cause because I had been assuming that we created the client object and then shared it around, when in fact we created a client during the processing of each job. I don't think there any actual issues in your code, but it turns out if you try to repeatedly create a Lowdown client, it doesn't do good things for your CPU. :)

​My co-worker was interested in writing a low weight APNS client, so we've switched to that until we have a chance to refactor our push notification code so that it shares a client.

​Thanks!

alloy commented 8 years ago

Ah ok, thanks for the update, though!

I now realise that maybe this was causing actors and their threads to keep running for you. If that was the case, it should now be gone too.