Compression broken in 3.0

[bennnjamin]

I am opening this issue now and will update as I dig into it further but so far I am not able to use compression at all since the 3.0 release. For now, disabling it has fixed the issue.

The way I'm enabling compression is like this:

$websocket = new Websocket(
    $server,
    $logger,
    new Rfc6455Acceptor(),
    $clientHandler,
    new Rfc6455ClientFactory(
        new Rfc7692CompressionFactory(),
        new PeriodicHeartbeatQueue(),
        null //disable rate limiting
    )
);

Sending the header Sec-WebSocket-Extensions: permessage-deflate; client_max_window_bits. from the client results in no extensions header response from the server. Just

HTTP/1.1 101 Switching Protocols.
connection: upgrade.
upgrade: websocket.
sec-websocket-accept: 6rnHuC9ZEBsJy6Cn5SHQyIGuzLE=.

I'm pretty sure the issue is caused by the introduction ofRfc6455Acceptor. Rfc6455ClientFactory sets the header and in testing, I can verify that code is executing and setting the header on client creation however the actual HTTP Response does not have the header in it.

[trowski]

Potential fix: https://github.com/amphp/websocket-server/compare/3.x...fix-compression

The compression context factory will also need to be passed to the acceptor.

[bennnjamin]

That fixes it, but there still seems to be an issue depending on the header sent by the client.

Working:

Sec-WebSocket-Extensions: permessage-deflate.

Not working:

Sec-WebSocket-Extensions: permessage-deflate; client_max_window_bits.

The difference in HTTP response is the presence of the Sec-WebSocket-Extensions header in the former case.

[trowski]

What is the header sent by the server in the case which isn't working?

[bennnjamin]

Full response below:

HTTP/1.1 101 Switching Protocols.
connection: upgrade.
upgrade: websocket.
sec-websocket-accept: y6cI8Vt16BtUOS53g14ZIVb6oeE=.

[trowski]

Ah, that's because the client_max_window_bits is requiring a value in Rfc7692Compression::fromHeader(), but it seems that value is optional. This will need fixing in the factory.

[bennnjamin]

I can reopen the issue there if you like. It seems the issue affecting this package is fixed in https://github.com/amphp/websocket-server/compare/3.x...fix-compression

[trowski]

Sure, it will remind me to take care of it.

FYI, after discussing this issue a bit with @kelunik we are considering deprecating the compression factory parameter to the client factory and not supporting compression with 3.0 and releasing a 4.0 with an interface which combines WebsocketAcceptor and WebsocketClientFactory.

[bennnjamin]

What would it take to help keep compression working for now? Refactoring interfaces is fine but I am getting some pretty big bandwidth savings on compression of JSON messages > 80KB and most of my clients are mobile on unreliable networks so minimizing bandwidth is important.

[trowski]

You can keep compression working for now by creating a WebsocketAcceptor class like Rfc6455Acceptor in the https://github.com/amphp/websocket-server/compare/3.x...fix-compression branch. Pass the same compression context to the acceptor and the client factory.

[trowski]

@bennnjamin I just tagged v4.0.0-beta.1 which refactors how compression is supported. The WebsocketCompressionContextFactory instance should be passed to the Websocket constructor instead of the client factory constructor.

Please give this new version a try and provide any feedback you may have. Thank you!

[bennnjamin]

Thanks I will take a look when I get a chance.

[trowski]

Tagged 4.0.0 with fixed compression support.