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

PushoverHandler cannot send multiple/ concurrent messages in quick succession #146

Closed
AlexButter opened this issue Jan 14, 2013 · 5 comments

Comments

@AlexButter
Copy link
Contributor

Hi!

We use the PushoverHandler, but found that sending messages to multiple users failed:

$logger->pushHandler(new PushoverHandler('app_key', 'user1_key', 'Error', self::CRITICAL));
$logger->pushHandler(new PushoverHandler('app_key', 'user2_key', 'Error', self::CRITICAL));

User 1 always receives the message, user 2 never receives the message.
Upon investigation of the problem, it looks like the stream is never closed, whilst the second call creates a new stream. Perhaps Pushover.net doesn't like this?

We found that posting the message using cURL solved the problem (being sure to curl_close() the channel. Here's the code we wrote to do so:

<?php

namespace acme\log\handlers;

use Monolog\Logger;
use Monolog\Handler\AbstractProcessingHandler;

class PushoverHandler extends AbstractProcessingHandler
{
    private $token;
    private $user;
    private $title;

    /**
     * @param string  $token  Pushover api token
     * @param string  $user   Pushover user id the message will be sent to
     * @param string  $title  Title sent to Pushover API
     * @param integer $level  The minimum logging level at which this handler will be triggered
     * @param Boolean $bubble Whether the messages that are handled can bubble up the stack or not
     */
    public function __construct($token, $user, $title = null, $level = Logger::CRITICAL, $bubble = true)
    {
        parent::__construct($level, $bubble);

        $this->token = $token;
        $this->user = $user;
        $this->title = $title ?: gethostname();
    }

    protected function write(array $record)
    {
        curl_setopt_array($ch = curl_init(), array(
            CURLOPT_SSL_VERIFYPEER => false,
            CURLOPT_URL => 'https://api.pushover.net/1/messages.json',
            CURLOPT_POSTFIELDS => array(
                'token' => $this->token,
                'user' => $this->user,
                'message' => substr($record['message'], 0, 512 - strlen($this->title)),
                'title' => $this->title,
                'timestamp' => $record['datetime']->getTimestamp(),
            ),
        ));
        curl_exec($ch);
        curl_close($ch);
    }
}

Perhaps closing the stream connection in the original handler code also solves this problem, but we didn't test that.

Regards,
Alex

@Seldaek
Copy link
Owner

Seldaek commented Jan 14, 2013

Could you try adding this in the original PushoverHandler and see if it fixes it?

    public function write(array $record)
    {
        parent::write($record);
        $this->closeSocket();
    }

This would close the socket after every call instead of keeping it open.

Another thing to try would be to add $this->setPersistent(true); to the constructor of the PushoverHandler, that way maybe it would just reuse the same socket for the two handlers, but I'm not sure.

On a sidenote, doesn't the pushover api allow sending a message to more than one person at once? That would be a better option :)

@AlexButter
Copy link
Contributor Author

I'll try those two suggestions tomorrow and let you know if they work out.
As for the pushover api itself, it only works for one user per push. I'll
see if I can submit a ticket to them to enable multiple users on a push, I
agree that would be a better option!

On 14 January 2013 18:24, Jordi Boggiano notifications@github.com wrote:

Could you try adding this in the original PushoverHandler and see if it
fixes it?

public function write(array $record)
{
    parent::write($record);
    $this->closeSocket();
}

This would close the socket after every call instead of keeping it open.

Another thing to try would be to add $this->setPersistent(true); to the
constructor of the PushoverHandler, that way maybe it would just reuse the
same socket for the two handlers, but I'm not sure.

On a sidenote, doesn't the pushover api allow sending a message to more
than one person at once? That would be a better option :)


Reply to this email directly or view it on GitHubhttps://github.com//issues/146#issuecomment-12229116.

@AlexButter
Copy link
Contributor Author

Using the setPersistent() method doesn't work, as only the first message is received.
Not sure why, but probably to do with the SSL/ HTTPS connection (even though pushover.net does send a Connection: keep-alive header).

Calling closeSocket() works, but only for the pushover.net application's owner because the PushOver API only allows messages to other users than the application's owner when connecting over SSL.

So, the fix for PushoverHandler.php:37 is:

parent::__construct('ssl://api.pushover.net:443', $level, $bubble);

instead of:

parent::__construct('api.pushover.net:80', $level, $bubble);

So the solution to our specific problem is to close the socket on every push while connecting over SSL.

Be aware that setPersistent() and SSL might be mutually exclusive...

@Seldaek
Copy link
Owner

Seldaek commented Jan 15, 2013

OK thanks for trying. I think using ssl + closeSocket() is a better approach than curl because curl isn't always available (neither is openssl, but I think it's more common). If you would like to submit a pull request with this approach it'd be great.

If you get any news from their side about multi-user-push please update this issue.

AlexButter added a commit to AlexButter/monolog that referenced this issue Jan 16, 2013
… connection after sending a message to PushOver.

Solves Seldaek#146: When sending messages in rapid succession to pushover.net only the first one is sent.
The SSL option is needed when sending messages to users that are not the pushover.net app owner; pushover.net doesn't accept those messages over plain HTTP.
@AlexButter
Copy link
Contributor Author

Closing this, see #148

mkraemer pushed a commit to Mercoline/monolog that referenced this issue Feb 4, 2013
… connection after sending a message to PushOver.

Solves Seldaek#146: When sending messages in rapid succession to pushover.net only the first one is sent.
The SSL option is needed when sending messages to users that are not the pushover.net app owner; pushover.net doesn't accept those messages over plain HTTP.
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