xmppo / xmpp-php

PHP client library for XMPP (Jabber) protocol
https://github.com/xmppo/xmpp-php
MIT License
45 stars 23 forks source link

re: 15/20 seconds to send a message #7

Closed jacksnodgrass closed 4 years ago

jacksnodgrass commented 5 years ago

I saw issue #6... but not sure I fully agree with the solution. Other php clients don't have the delay. If you set the timeout to 0 ( it is set to 1 ) in: src/Socket.php

    /**
     * Period in seconds during which the socket will be active when doing a socket_read()
     */
    protected $timeout = 0;

    /**
     * Socket constructor.
     * @param Options $options
     * @throws DeadSocket
     */
    public function __construct(Options $options)

There is no delay in sending the message.

There is an error in: src/Buffers/Response.php ... but this:

    public function read()
    {
        if(!empty($this->response)) {
                $implodedResponse = implode('', $this->response);
                $this->flush();
                return $implodedResponse;
        } else {
                return $this->response;
        }
    }

corrects thost issues.... I don't know if this will cause issues down the line or not.

It seems like AS-IS that

    /**
     * Period in seconds during which the socket will be active when doing a socket_read()
     */
    protected $timeout = 1;

IMPOSES a 1 second delay on a socket send/read and that's not what the timeout stuff usually does. If you change that to 10.. it waits 10 seconds after each send... timeout is supposed to say wait UP TO that many seconds... not wait that many seconds.

Norgul commented 5 years ago

Hello Jack,

thank you for the analysis. Reading the PHP documentation on setting the socket timeout I would never have guessed that it is imposing a timeout instead of limiting it.

The issue with reducing it to zero though is that with establishing TLS sessions I absolutely need to wait for the server response. Response get's parsed to see whether TLS is required, and if it is, it starts a secure connection. With timeout being zero, it just skips over it as I guess now it executes the auth part "too fast" so it doesn't get to analyse the response at all...breaking the session to early as it couldn't authenticate.

I will take a look into it.

jacksnodgrass commented 5 years ago

Think you can add: stream_set_blocking($this->connection, false); before the stream_set_timeout($this->connection, $this->timeout); AND allow for a null string to be returned in:

    public function getResponse(): string
    {
        $this->socket->receive();
        $response = $this->socket->getResponseBuffer()->read();
        if($response) {
                $finalResponse = $this->checkForErrors($response);

                return $finalResponse;
        }
        return "";
    }
Norgul commented 5 years ago

checkForErrors() function already handles cases where no response is available, so that last part is not needed.

If I set timeout to 0 and set socket to be either blocking or non-blocking, the same issue happens.

The thing is that I need to wait for server response before I send the next stanza. Setting the timeout to 0 doesn't wait for it, but instead sends stanzas much faster than server can obviously get and return them.

Were you actually able to make it work with your server or?

jacksnodgrass commented 5 years ago

I got it to work with my change but I think it was to a server with out ssl. It did not work on another server. I introduced a usleep(500); in Socket.php send() just before it did the $this->receive(); which seemed to help with my ssl enabled server ... but still think that it should be possible to do this without a usleep().

Norgul commented 5 years ago

I did try using sleep as well. It does the trick, but what is the purpose of setting timeout to 0 if you are going to sleep it for the same amount instead?

I am testing out different options and solutions though. Will keep you informed if I get to something

jacksnodgrass commented 5 years ago

usleep(250) or usleep(500) seems to work... and that's 1/2 to 1/4 the wait of a full second. Not saying that's the right answer but it works better than the 1 second delay currently done.

Norgul commented 5 years ago

I get that, but what I meant is that you could as well set timeout's second parameter (seconds) to zero and third to 500. But sure, quicker for sure

Norgul commented 4 years ago

Hello @jacksnodgrass, I've pushed the changes with speed improvements to new patch version. Closing as resolved. Try it out and tell me what you think. Thanks