Added it to both 8.0 and 8.1. It's in use in my Socket.IO implementation, and so far it passes all tests.
Since testing it involves using a webbrowser as counterpart, my tests are sadly not comprehensive.
Arne, could you check the code and see if it meets your criteria on the proposed visible interfaces?
There is a little bit of trickery involved in the rsv bits. In order to know if to decompress, we need to know the RSV1 bit on the *first* frame part of the larger frameset.
Cool! I have been using the 'autobahn' testsuite recently. I will check against it, supports that extension.
Arne
On 09/26/16 14:15, Stephen R. van den Berg wrote:
Added it to both 8.0 and 8.1. It's in use in my Socket.IO implementation, and so far it passes all tests.
Since testing it involves using a webbrowser as counterpart, my tests are sadly not comprehensive.
Arne, could you check the code and see if it meets your criteria on the proposed visible interfaces?
There is a little bit of trickery involved in the rsv bits. In order to know if to decompress, we need to know the RSV1 bit on the *first* frame part of the larger frameset.
General remarks/questions with regard to the information being stored in class Frame:
I see that you usually tend to separate out different bit-flags into their individual pike member variables of type int. Case in point: fin, rsv1, rsv2, rsv3 and mask.
Why don't you choose something closer to the wire-format where you use:
int flags;
enum { FIN = 0x80, RSV1 = 0x40, RSV2 = 0x20, RSV3 = 0x10 }
and then test for fin using: flags & FIN etc. ?
It's not like we require an ultra-userfriendly API to the basic frame flags. It just needs to efficient and readable.
I think there is no particular reason for this, just that this code grew 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 free to reduce it to one flags field in the Frame class and add getters for compatibility.
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.
Arne
On 09/26/16 15:58, Stephen R. van den Berg wrote:
General remarks/questions with regard to the information being stored in class Frame:
I see that you usually tend to separate out different bit-flags into their individual pike member variables of type int. Case in point: fin, rsv1, rsv2, rsv3 and mask.
Why don't you choose something closer to the wire-format where you use:
int flags;
enum { FIN = 0x80, RSV1 = 0x40, RSV2 = 0x20, RSV3 = 0x10 }
and then test for fin using: flags & FIN etc. ?
It's not like we require an ultra-userfriendly API to the basic frame flags. It just needs to efficient and readable.
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.
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.
Arne
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.
Arne Goedeke wrote:
- permessage-deflate is activated by default. i don't believe that
this is a good choice.
The default is what should help most users. I think it would have, but it's not a big deal.
- 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!
I tried to follow the standard here, but I admit that the different conditions spelled out in the standard are convoluted to begin with.
- 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.
Ah, I misread the standard here, I think.
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.
I'll have a look. I'm in the middle of some SocketIO/EngineIO refactoring as well.
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.
Ok, fair enough. I'll remove it again (minus minor fixes).
On 10/18/16 11:46, Stephen R. van den Berg wrote:
Arne Goedeke wrote:
- permessage-deflate is activated by default. i don't believe that
this is a good choice.
The default is what should help most users. I think it would have, but it's not a big deal.
My applications for websockets usually focus on binary data. I usually care more about latency and CPU than bandwidth and the payload does not compress well. This is probably different for situations which involve text. I don't know what other people do. I would propose the following: Let's wait until this extension support has become more stable and then decide which of the "extensions" make sense as a default (defragment is probably helpful too in most situations).
- 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!
I tried to follow the standard here, but I admit that the different conditions spelled out in the standard are convoluted to begin with.
I just extended the extension support to client mode connections. That seems to work fine so far. The client does not currently offer any of the extension parameters during handshake.
To implement the handshake correctly we have to:
* fail connections with invalid parameters * implement fallback (clients can offer several combinations of parameters and a server is supposed to pick the first that works)
- 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.
Ah, I misread the standard here, I think.
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.
I'll have a look. I'm in the middle of some SocketIO/EngineIO refactoring as well.
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.
Ok, fair enough. I'll remove it again (minus minor fixes).
Thanks.
On Wed, Oct 19, 2016 at 11:20 PM, Arne Goedeke el@laramies.com wrote:
On 10/18/16 11:46, Stephen R. van den Berg wrote:
Arne Goedeke wrote:
- permessage-deflate is activated by default. i don't believe that
this is a good choice.
The default is what should help most users. I think it would have, but it's not a big deal.
My applications for websockets usually focus on binary data. I usually care more about latency and CPU than bandwidth and the payload does not compress well. This is probably different for situations which involve text. I don't know what other people do.
It'd have to be decent quantities of text. I have a D&D battle grid that could benefit from websocket support (it currently uses long polling). The messages would be text, but a single line at a time, and generally no more than 20-30 characters. Compression isn't going to materially affect those - they'll be carried in a single packet either way.
+1 on sitting tight and waiting for more info.
ChrisA
Arne Goedeke wrote:
On 10/18/16 11:46, Stephen R. van den Berg wrote:
Arne Goedeke wrote:
- permessage-deflate is activated by default. i don't believe that
this is a good choice.
The default is what should help most users. I think it would have, but it's not a big deal.
My applications for websockets usually focus on binary data. I usually care more about latency and CPU than bandwidth and the payload does not compress well. This is probably different for situations which involve text. I don't know what other people do.
Well, I cater/catered for that. I.e. binary frames are mostly not compressed, even if compression is on. Text frames are mostly compressed. I tried to make it adapt favourably to most real-life conditions, including yours.
I would propose the following: Let's wait until this extension support has become more stable and then decide which of the "extensions" make sense as a default (defragment is probably helpful too in most situations).
Sounds reasonable. Yes, defragment should have been in there from the get-go. I was surprised it wasn't in the 8.0 interface. It doesn't really offer any advantage on the application level to assemble the fragments yourself.
I just extended the extension support to client mode connections. That seems to work fine so far. The client does not currently offer any of the extension parameters during handshake.
- fail connections with invalid parameters
- implement fallback (clients can offer several combinations of
parameters and a server is supposed to pick the first that works)
Yes, I know. That's the reason I skipped that, because doing the client was more work than the server side.
Just in case someone wants to use the 'autobahn-testsuite', I have published two simple pike scripts which serve as corresponding client/server endpoints here:
https://github.com/arneg/pike-autobahntestsuite
Arne
On 10/18/16 11:29, Arne Goedeke wrote:
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.
Arne
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.
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.
These kinds of micro-optimizations are better addressed in the langauge itself. Unless the programmer knows Pike internals they are often not effective.
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.
Pike 8.0 is the stable branch and releases are attempted once a month. Only put things in 8.0 that you are happy to be released in the state you check them in.
pike-devel@lists.lysator.liu.se
-
Arne Goedeke -
Chris Angelico -
Martin Nilsson (Coppermist) ＠ Pike (-) developers forum -
Stephen R. van den Berg