not completed exception handling can cause total crash

[supipd]

Application, handled websockets is supposed to be stable even in case of some problems on client side. Huge using exceptions without cath-try handling cause crashing.

For (first look) example Connection.__construct calls configureClientInformation with potential exception without try-catch. If new client connects, and invoke exception , all application crashes and all actually connected innocent clients have problem.

I have long-running PHP workers with monitoring possiblity through web sockets. Newest version (2.0) of php-websockets (protocol 13) cannot be used without big risk or big rework.

How to correctly handle exceptions ?

[dominics]

Hi @supipd

That's a really good point about the connection exception not being handled correctly. It's probably best for the ConnectionManager to handle exceptions thrown in the socket constructor: it is responsible for the instantiation in the first place.

In handleClientSocket, we correctly have the connection processing wrapped in exception handlers: https://github.com/varspool/Wrench/blob/master/lib/Wrench/ConnectionManager.php#L235 - but we don't yet do so in handleMasterSocket, which handles accepting new connections. Specifically, line 193 ($connection = $this->createConnection($new);) ought to be moved into the try/catch above it.

[supipd]

HI dominics,

maybe it's a more general problem ... exceptions or return values, but in php-websockets code are lot of usefull public function, which can be used alone f.e. in registered applications.

One thing is correctly handle possible exceptions in main execution code (ConnectionManager),

Other thing is using public functions (f.e. getNamePart, getPayload ... ) in registered application code, where man must very carefully study called functions for potential exceptions.

EXAMPLE:

class ServerCrashApplication extends Application
{
    public $whitelist = array( "127.0.0.1","2.2.2.2" );
    public $blacklist = array();    // f.e.: "1.2.3.4:1212"
    protected $clients = array();
    public function onConnect($client)
    {
        for ($i = 0, $bl = false; $i < count($blacklist); $i++) {
            // any mistake in IP:PORT format -> exception -> MUST CATCH-TRY
            if (    $client->getIP() == $client->getNamePart($blacklist, wsSocket::NAME_PART_IP))
                &&  $client->getPort() == $client->getNamePart($blacklist, wsSocket::NAME_PART_PORT))
            ) {     // simple return f.e. false corectly run without needing of CATCH-TRY
                $bl = true;
                break;
            }
        }
        if (! $bl) {
            $this->clients[] = $client;
        }
    }
    public function onData($data, $client)
     {  // special client can add (any, even incorrect) item to blacklist !!!!!!!!!! ONLY EXAMPLE 
        if ( in_array($client->getIP(), $whitelist ) {
            $blacklist[] = $data;   // what if data are not correct IP:PORT ??? check, CATCH-TRY...
        }
        $client->send(date('d-m-Y H:i:s')." ".$data);
    }
}

Yes, yes, I can use try ... catch blocks almost on every line ...

I tried use wrapper function like this:

$central_error_msg = "";
define('NEVER_KILL', true); //false );
function handle_throw( $exc , $retval = false, $force = false) {
    $central_error_msg = $exc->message;
    if (! NEVER_KILL || $force) {
        throw $exc;
    }
    return $retval;
}
/*
    ....
    //  throw( new wsSocketException('Could not parse name parts: ' . $name);
    return handle_throw( new wsSocketException('Could not parse name parts: ' . $name));
    ...
*/

but it needs detailled studies all source code for correct retval's and maintain it ... maybe FORK, but it is hard way.

[supipd]

Sorry for CLOSED - unintentional

[dominics]

This would be less of a problem in a language where thrown exceptions are enforced by the compiler.

I agree that it's not so nice that a Connection will sometimes throw exceptions when simply requesting the details of a connected client. But, unfortunately, I'm not collecting those details until you request them, and at that point, stream_socket_get_name() (http://php.net/stream_socket_get_name) is liable to fail in some instances (for instance, if the client is no longer there).

One possible enhancement would be to make Socket->getIp() and Socket->getPort() follow the contract you outlined and return false instead of an exception if the socket details cannot be determined. This would involve more error handling code whenever those methods are called, however, and it puts the onus on the caller to check in the first place.

Using the $options array on the ConnectionManager, you could configure a subclass of the socket_client_class, to do exactly this if you wanted to though; no code changes needed.

For your attached example, I would suggest that IP black/whitelisting would be better done in a subclass of the connection manager or server - check out the BasicServer, which adds a rate limiter, for an idea of how to extend Wrench in this way.

However, I think an IP address is a reasonable requirement to make of a socket object, and not being able to find it is an exceptional problem. But, I think the exception thrown by the Socket should be caught by the Connection, and the Connection should be closed (and onConnect() on the Application not called)

So, I think Wrench should attempt to parse the IP address and port when the client connects, and before turning it over to the Application. That means, however, that while before you could catch the exception and decide what to do, now, you wouldn't have a choice (on the other hand, when stream_socket_get_name fails, it usually indicates the underlying socket resource has gone away).

[dominics]

class NamedConnection extends Wrench\Connection
{
    protected function configureClientInformation()
    {
        try {
            parent::configureClientInformation();
        } catch (\Exception $e) {
            $this->log('Disconnecting unidentifiable client', 'warn');
            $this->close();
        }
    }
}

Then specify NamedConnection as the connection_class to the ConnectionManager. This will implement the second behaviour described above. Lose the handler, and everything should continue as normal, but the ->ip, ->port and ->id on the connection will be null.