phergie / phergie-irc-client-react

IRC client library built on React
BSD 3-Clause "New" or "Revised" License
56 stars 26 forks source link

QUIT handler in WriteStream results in the socket being closed before writing the QUIT command #32

Closed Renegade334 closed 9 years ago

Renegade334 commented 9 years ago

The quit handler in WriteStream intends to write the command to the socket and then close the stream as follows:

$this->emit('data', array($msg));
$this->close();

On calling $this->close(), the 'end' event is thrown and the closure returned by Client::getEndCallback will mop up and close the socket stream. This occurs before the socket stream handles the send buffer, so the quit command is never sent to the server, and other IRC users will see a disconnect message saying that the client closed the socket, rather than seeing the intended quit message.

There needs to be some mechanism whereby the socket stream's send buffer is flushed before closing it.

Renegade334 commented 9 years ago

Could possibly just be a case of changing $stream->close() to $stream->end() in Client::getEndCallback. Needs to be tested to see if that fails gracefully if the socket is a timed-out zombie, but my guess is that it should work.

Renegade334 commented 9 years ago

Appears to work. I set up an ircQuit timer, then severed the physical network connection before it was called. $stream->end() still resulted in the stream being closed instantly, and the IRC server registered it as a client timeout after a couple of minutes.

Testing it with the connection still active, on the other hand, resulted in the quit message is sent to the server correctly before the socket was closed.

elazar commented 9 years ago

@Renegade334 Awesome! Yeah, think this was just an oversight on my part. PR? :)

Renegade334 commented 9 years ago

Do the read and write streams for the connection need to be explicitly closed as well once the socket stream closes? At the moment, they're not, and from what I can see, there's no reason why they shouldn't be.

elazar commented 9 years ago

I believe React\Stream\Stream handles that when it's closed; see its end() and close() methods.

Renegade334 commented 9 years ago

I'm referring to the Client\ReadStream and Client\WriteStream objects, rather than the socket stream resource.

elazar commented 9 years ago

Ah, I think I see what you mean: close() is called on WriteStream within its ircQuit() method, but I don't think close() is ever called on the ReadStream instance. Maybe getEndCallback() should be modified to accept both streams and call end() on both?

elazar commented 9 years ago

Merged #34.