Refresh (reconnect) from IE crashes the server

[trungdinhtrong]

HI,

I tested the provided testwebsock.php with client.html and observed this on Internet Explorer 11:

• I ran the server srcript, testwebsock.php, on an apache server on ubuntu.
• I ran the client.html on Internet Explorer 11. The client successfully connected to the server. I could send a message and received the echo.
• When I clicked the "Refresh" button on IE, the server crashed, printing out this message: "Socket error: Connection reset by peer"

Any suggestion on why this might happen or how to fix/investigate?

Thanks

[trungdinhtrong]

I think the Exception was thrown here:

$numBytes = @socket_recv($socket,$buffer,$this->maxBufferSize,0); 
          if ($numBytes === false) {
            throw new Exception('Socket error: ' . socket_strerror(socket_last_error($socket)));
          }

I don't know what cause the IE to send the error 104 (Connection reset by peer) though. May be IE did not follow the closing protocol properly? But in general, should this error be handled somewhat more gracefully? E.g., in case of 104 error should we just close the socket (if I understand the error code correctly, the client basically told us that it is disconnected already) and continue?

Please let me know if I am wrong. Thanks

[Xaraknid]

First, the http server you use is irrelevant because php-websocket is a standalone server using cli ( command line interface).

When you refresh did you see IE reconnect?

Simply because when you refresh with IE it's like "slamming the phone back on the hook". Not polite close session "using quit". It's normal behavior this error is sent when the connection did not say goodbye to each other.

I'm not sure but I think you are right throwing execption break the loop you need to keep server alive.

try replacing this with :

 if ($numBytes === false) {
        $this->stdout('Socket error: ' . socket_strerror(socket_last_error($socket)));
    }

[trungdinhtrong]

Thanks Xaraknid. I have somewhat the same thought about slamming the phone. Though it should be noted that same thing does not happen to Firefox, Opera, and Chrome (the browsers I tested).

Regarding keeping the server alive, I am thinking of this:

 if ($numBytes === false) {
            $err_num = socket_last_error($socket);
            if ( $err_num == 104) {
               socket_clear_error($socket);
               $this->disconnect($socket);
               $this->stdout(socket_strerror($err_num));
            } else {
                throw new Exception('Socket error: ' . socket_strerror(socket_last_error($socket)));
            }

Basically I think in case of error 104 we probably should close the socket (because the remote site already disconnected) and generally we also need to clear the error (because the document of socket_last_error indicates that the error will not be cleared unless we call socket_clear_error).

I have not read all other errors, so I dont know if it is always save to continue, though.

[Xaraknid]

There are standard used by Firefox, Opera and Chrome and IE alone in other side of the ring. Most probably in first team when you refresh they close gracefully any connection before refresh it.

The problem is not the error but ANY error throw WILL break the loop causing server to stop. Most probably cause by how exception work in php. So we should maybe not use try...catch for the server.

try {
  $echo->run();
}
catch (Exception $e) {
  $echo->stdout($e->getMessage());
}

replace it by

$echo->run();

There is no need to clear error as you only call socket_last_error when error occur and you will see the last error.

EDIT : after some testing. I be able to reproduce this error not happens everytime for me only when I spam "reconnect".

There is no point checking error 104 and doing the cleaning job ( calling disconnect ) because the server allready will deal with that :

Client #2 connected. Resource id #15

Warning: socket_recv(): unable to read from socket [104]: Connection reset by peer in /home/xaraknid/srv/core/websockets.php on line 83
Socket error: Connection reset by peer
Client disconnected. Resource id #15
Client disconnected. TCP connection lost: Resource id #15

As you can see it's been catch by "elseif ($numBytes == 0) {" on next loop. Unless you want to log it or see who slammed the line there is no need to worry about that.

[ghedipunk]

If you don't have the try/catch block, it'll throw a fatal exception if it hits the throw statement. Before this, it was the same results on any socket errors: end of execution of the run() method.

I just pushed a fix up to the legacy dev branch, but I don't have my development server in a stable state tonight, so I can't do a smoke test.

It should cover each case where just one client is disconnected or fails to fully connect and the issue is at the Data Link, Network, or Transport layers.

[Xaraknid]

You are right try/catch is needed if you use throw statement. But I think using throw statement is not a good idea because any throw end the execution of run() method. It's good if the server only wait for only one client ( you ).

Only need to drop connection to current socket not end the whole server. It's my tought that's why I did not think try/catch and throw should be used.

[amb3rl4nn]

Agreed about preserving the server and limiting the termination to the socket only. It's not uncommon to have socket failure of some kind with large connection pool and many clients. Cleanly removing them and continue to process new/current connections is key. On May 12, 2015 1:53 AM, "Xaraknid" notifications@github.com wrote:

You are right try/catch is needed if you use throw statement. But I think using throw statement is not a good idea because any throw end the execution of run() method. It's good if the server only wait for only one client ( you ).

Only need to drop connection to current socket not end the whole server. It's my tought that's why I did not think try/catch and throw should be used.

— Reply to this email directly or view it on GitHub https://github.com/ghedipunk/PHP-Websockets/issues/32#issuecomment-101140836 .

[ghedipunk]

Removed throwing the exception. Still untested and is on dev-legacy.

Once tested, I'd like to merge into master, then merge master into legacy.

[trungdinhtrong]

Hi ghedipunk,

Your switch statement makes sense to me, and testing confirm it would fix the 104 error (it should fix all those disconnecting errors that you included also). However, I receive the following warning message when I reset IE (to reproduce the 104 error):

PHP Warning:  socket_clear_error(): 9 is not a valid Socket resource in /home/xxx/phpwebsocket/websockets.php on line 89

I believe it is because you disconnect before clearing error. I think the disconnect function would close the socket and hence when you call socket_clear_error you give it an already closed socket. The problem should be fixed if you clear the error before closing the socket. On the other hand, I am wondering if we actually have to clear the error at all since closing the socket would "clear" the error anyway? To be safe I am inclined to clear it, but you were right in your comment that someone might implement their own check for error condition on the socket. For a longer term solution, may be we should consider adding a call back explicitly for error handling?

[ghedipunk]

Moved the socket_clear_error() line down into disconnect(). Once the socket is closed the error wouldn't need additional explicit clearing.

Added an optional parameter to the disconnect() method, to allow a developer to get the actual error that is triggering the disconnect.

[ghedipunk]

Current behavior when refreshing with IE:

Client connected. Resource id #7
frame #1 position : 0 msglen : 2 + headers_size 6 = framesize of 8
########    PACKET END         #########
Unusual disconnect on socket Resource id #7
Client connected. Resource id #8
frame #1 position : 0 msglen : 2 + headers_size 6 = framesize of 8
########    PACKET END         #########
Unusual disconnect on socket Resource id #8
Client connected. Resource id #9
frame #1 position : 0 msglen : 2 + headers_size 6 = framesize of 8
########    PACKET END         #########
Unusual disconnect on socket Resource id #9
Client connected. Resource id #10

I'm happy with the results. I'm merging dev-legacy into master, then master into legacy.

Leaving open for a day in case anyone else has problems.

[trungdinhtrong]

Thank you all again for the fix and the discussion.

[Xaraknid]

Just to let you know it's not required to send a close request to client who have allready hang-up the line. In fact there's no need at all to implicitely call disconnect because the server allready catch the windowed connection with :

elseif ($numBytes == 0) {
   $this->disconnect($socket);
   $this->stderr("Client disconnected. TCP connection lost: " . $socket);
} 

There a great idea by @trungdinhtrong :

For a longer term solution, may be we should consider adding a call back explicitly for error handling?

If we add an abstract onerror($user,$sockerror) the dev can deal the way he want with the error with client. As example client reset by peer can mean the client never recieve the message so you can save it and send it back when he come back. And we could eventualy use this method to send internal php-websocket error too.