I have finally found some time to actually test the new extension
support. Unfortunately, it breaks several test cases. Problems I found:

* permessage-deflate is activated by default. i don't believe that
  this is a good choice.
* the negotiation code which determines the final set of options is
  faulty. For instance, it activates the deflate extension even if it
  was not actually negotiated. This breaks websockets for all clients
  which do not use this extension!
* the extension itself is defined on messages not frames. this is why
  low_parse (for instance) is not necessarily the best place to do the
  inflate() and one reason why it does not work correctly at this point.

I initially started by fixing the first two problems but then decided
that solving the third problem will require some refactoring. I
therefore added a new API which allows implementing extensions using
classes. I refactored your code to use that new API. The code now passes
the complete 'autobahn-testsuite', including all tests for
permessage-deflate. The code has lost the per-frame option overwrite for
deflate, but I am sure we can fit that back in. Please check if it works
with your setup.

The same extension API is now also used to implement defragmentation and
conformance checks. None of the extensions is used by default, they need
to be explicitly specified in calls to websocket_accept(). The client
side code does not currently support them but I will add that later in 8.1.

I do not intend to backport this to 8.0 since the code has already
diverged quite a lot. Additionally, this introduces a new API, which
might change again in 8.1. I therefore stand by my initial comment. This
should not have been added to 8.0 and I think it should be removed again.


On 09/26/16 19:54, Stephen R. van den Berg wrote:
> Arne Goedeke wrote:
>> over time. My general feeling about performance in Pike is also that in
>> many cases these things do not make a huge difference. However, feel
> True.  Then again, I'm planning on having large Pike farms serving
> hundreds of Socket.IO clients/browsers.  Then performance starts to matter.
>> I have a general comment about adding things to pike 8.0. If you change
>> existing code it means that those additions somehow need be properly
>> tested before a release can be made. I think it makes more sense to add
>> those changes to 8.1 first and backport them when they have been found
>> to be sufficiently stable to be included in a _stable_ release. It also
>> allows letting the APIs and conventions settle without shipping that to
>> users. I know that there are no official rules about this, but I think
>> it makes life alot easier.
> Well, I agree, obviously.
> The issue here is clouded a bit by the following factors:
> - I discovered that WebSocket support in Pike 8.0 is a bit flaky here and
>   there, so in all likelihood, not many people are using it in its current
>   8.0 form.
> - In order to get it working, I ended up fixing parts of it first.
> - I'm trying to run a Socket.IO farm on a production system, so I'd like
>   the platform to be Pike 8.0 instead of 8.1.
> - I didn't expect to fix this much.  I'd had hoped that compression
>   was already supported by the current implementation.
> - The EngineIO and SocketIO stuff is completely new, so no backwards
>   compatibility issues.  I expect the API to settle within a few days,
>   because then I've built a running application with it.
> All that said, if a Pike 8.0 release needs to be done and the code is not
> deemed sanctioned/stable yet, I'll happily (temporarily) strip it back out
> again, and then reschedule putting it back in at some later point in time.

Reply via email to