debug-ito / AnyEvent-WebSocket-Server

WebSocket server for AnyEvent
7 stars 2 forks source link

Implemented adding parameters for AnyEvent::Handle to the establish m… #4

Open TheAthlete opened 5 years ago

TheAthlete commented 5 years ago

Implemented adding parameters for AnyEvent::Handle to the establish method

This functionality is required to add various AnyEvent::Handler parameters and handlers, for example:

use AnyEvent::Socket qw(tcp_server);
use AnyEvent::WebSocket::Server;

my $server = AnyEvent::WebSocket::Server->new();

my $tcp_server;
$tcp_server = tcp_server undef, 8080, sub {
    my ($fh) = @_;
    $server->establish($fh, {
      on_eof => sub {
        my $handle = shift;
        # some code
      },
    })->cb(sub {
        my $connection = eval { shift->recv };
        if($@) {
            warn "Invalid connection request: $@\n";
            close($fh);
            return;
        }
    });
};
debug-ito commented 5 years ago

Thanks for making the pull-request.

I don't mind adding capability to customize the AnyEvent::Handle object, but that feature would be experimental forever. Changing configuration of the AnyEvent::Handle object changes its behavior drastically, and I cannot guarantee AnyEvent::WebSocket::Server still works correctly. In addition, some attributes (e.g. on_read and on_eof) of AnyEvent::Handle are overwritten by AnyEvent::WebSocket::Server and AnyEvent::WebSocket::Connection modules. Is it ok for you?

If you describe a little more detail on what you want to do, maybe we can come up with a more reliable way to achieve that.

s0me0ne-unkn0wn commented 5 years ago

@debug-ito What we're trying to achieve here is to take the control over the situation when connection closes unexpectedly during websocket handshake. Without the patch, we had sporadic crashes inside the handshake code which we couldn't handle (I assume it happened when clients were closing the connection before handshake procedure was complete, although we didn't investigate it in deep). After the patch provided by @TheAthlete those crashes are gone, that's why I believe it's useful. Any advice on how to make it in a right way is appreciated!

debug-ito commented 5 years ago

Thanks for explaining your problem.

If something bad happens during the handshake process, it should be reported as error from the AnyEvent::CondVar returned by establish method. If the current implementation fails to do that, it's a bug and should be fixed so that the CondVar reports the error.

Did the CondVar throw any exception when $condvar->recv is called? What error message did you get?