permessage-deflate server implementation #110

wishdev · 2021-10-19T05:00:35Z

This is a starting point to allowing for an Iodine WebSocket Server connection to support permessage-deflate.

The first couple of patches are housecleaning for Ruby 3.0+ compat.

I added 2 dev facing changes

To turn on the feature one uses env['rack.upgrade.deflate'] = true alongside the current env['rack.update'] to set the handler
Connection#write would accept a :deflate = false keyword parameter to turn off deflating for a particular message. For example, if you are sending an already compressed object - this would prevent a second compression for no real purpose

No tests or documentation - wanted to throw it over the wall to see if there was any major issues with the methodology before going tooooo far.

boazsegev · 2021-10-19T22:09:52Z

Hi @wishdev ,

That's some wonderful writing you put in, thanks! 👏🏻👏🏻👏🏻

As for the practicalities:

I love that we are updating the C layer - though that should probably go as a PR into the facil.io repo.
I love the use of the int deflate, though I think it should be a non-boolean implementation (i.e., deflate if message length is over X bytes). Also, maybe ssize_t would work better for that.
However... I don't really like the API changes, as they burden the developer with transept layer information, which IMHO should be abstracted away whenever possible.

This is my rational (please feel free to argue with this):

Part of the facil.io and iodine design philosophy is that the framework should require zero HTTP / WebSocket knowledge from developers. The API could be used to write to a local file / string as instead of a WebSocket (or SSE) connection. Although Rack requires some familiarity with HTTP, iodine minimizes the need (for example, it tests for and adds missing headers such as content-length and date).

I would avoid the addition of the rev field in the websocket_write function. Deflate messages only if they are text messages over X bytes long (where X is the value of int deflate in the WebSocket settings). Binary messages should assume to have their own compression (user side implementation).

Yes, this means a developer cannot explicitly avoid deflation for pre-deflated text messages... but if a developer really wants to get into the nuts and bolts, they can exchange binary messages with user implemented deflation/inflation.

What do you think?

Thanks again!
Bo.

boazsegev

This is my preliminary review. I was speed reading and didn't get to review everything just yet, however, the main idea behind the requested changes is to protect the existing separation of concerns.

The iodine server (facil.io framework) should handle the WebSocket protocol details. The user (web app developer) shouldn't have any knowledge of the WebSocket protocol or its details.

boazsegev · 2021-10-20T00:13:59Z

ext/iodine/http1.c

@@ -349,6 +349,10 @@ static int http1_http2websocket_server(http_s *h, websocket_settings_s *args) {
  stmp = fiobj_obj2cstr(tmp);
  fiobj_str_resize(tmp,
                   fio_base64_encode(stmp.data, fio_sha1_result(&sha1), 20));
+
+  if (args->deflate) {
+    http_set_header(h, HTTP_HEADER_WS_SEC_EXTENSIONS, fiobj_dup(HTTP_HVALUE_WS_DEFLATE));


In addition to testing the deflate setting, this should be automated to test for the permessage-deflate value in the Sec-WebSocket-Extensions header.

Web app developers shouldn't have to know anything about the permessage-deflate WebSocket extension. The choice to enable or disable deflate support should be limited to the settings where the value 0 is reserved to be replaced with a "smart" default and the "disabled" value should be -1.

I believe you are mistaking what the deflate variable within websocket_settings_s does (at least currently).

When a connection is upgraded - there needs to be something to store whether or not we want to send out the sec-websocket-extensions: permessage-deflate header. That is the job of deflate within websocket_settings_s.

It also informs the iodine websocket struct later on via the websocket_attach method within websockets.c

Yes, there is a place for global settings, it sets the HTTP and WebSocket behavior. For example, it limits WebSocket message sizes to avoid memory drain attacks on the server. This is usually set by the CLI (iodine_cli_parse) or the Iodine.listen method which sets the value in the iodine_http_listen function.

I believe this setting should definitely be settable using the CLI where current behavior should be preserved by setting -ws-deflate = -1.

I also believe this value should be a numerical value representing the minimum length a text message would have to be before it is compressed. Wasting CPU cycles to compress a 16 byte long message (i.e. `""updates":{}") would be regrettable.

boazsegev · 2021-10-20T00:17:08Z

ext/iodine/iodine_connection.c

+    CONST_ID(keyword_ids[0], "deflate");
+  }
+
+  rb_scan_args(argc, argv, "1:", &data, &opts);


This should not go in the developer API. This part should be automated according to the WebSocket settings and the value of the Sec-WebSocket-Extensions sent by the client.

This merely allows for a developer to turn off the deflate at an individual message level. It is most likely overkill - but it wasn't rocket science to deploy (assuming rsv floats down the food chain a little). More than happy to remove.

However, to be clear, a developer never needed this to deflate something - the default was nil and nil made no change to the connection setting - so if the connection deflated messages - they would be deflated without any developer intervention.

I will note that making this change would allow for pub/sub messages to be deflated immediately as well - I did not hook up this change to those methods yet.

I think it's an overkill and adds complexity to a function that might be called fairly often. I'm not a performance junkie, but I think it's better to avoid adding complexity to this code path.

boazsegev · 2021-10-20T00:26:21Z

ext/iodine/iodine_connection.c

+      rsv = 4;
+    }
+
+    websocket_write(socket, IODINE_RSTRINFO(data),


Please automate the rev value within the websocket_write function, not as an argument.

Same thing as the last note - no issue to remove this if the per-message kill option is overkill.

boazsegev · 2021-10-20T00:52:48Z

ext/iodine/iodine_http.c

+  VALUE extension_header = rb_hash_aref(env, RACK_WS_EXTENSIONS);
+  char *extensions = (extension_header == Qnil ? NULL : StringValueCStr(extension_header));
+  if (extensions != NULL && strcasestr(extensions, "permessage-deflate") != NULL &&
+      rb_hash_aref(env, RACK_UPGRADE_DEFLATE) == Qtrue) {


I think it is better to remove the 'rack.upgrade.deflate' option, or make it so the default value follows the settings.

Honestly I don't see a use case for excluding some connections rather than others. If needs must be, the same can be achieved by using binary transport vs. text transport and excluding binary transport from the extension.

So I think this is where I'm confusing things.

There are no global settings here for deflation. I don't know where I would hook into to get that available (if something exists please let me know).

The workflow here is pretty straight forward. We're inside the websocket upgrade handshake and all I have is a standard iodine http connection. I don't have a iodine websocket connection yet. So I need something to tell facil.io to send out the deflate header when it upgrades the connection via http_upgrade2 call (just below this note).

The first two lines check for and retrieve the inbound Sec-WebSocket-Extensions header from the rack environment.

Then the if statement make 3 checks

We have the inbound header

The inbound header contains permessage-deflate

That we set rack.upgrade.deflate to true for the connection

So there is a check for the inbound header and it is here.

As far as rack.upgrade.deflate being removed. There is no reason why it needs to be there. I, however, really hate modifying default behavior for working code based on an extension to a concept. There is no default need for deflate and while I certainly want it myself - I'm not going to code a change without being really sure that's the right call for someone with currently working code.

The default could be changed to true very easily as a simple alternative as well.

I'm not certain that leaving something like rack.upgrade.deflate as an option for someone that really needed it hurts here. It's a single check at the same point I would make the check for the inbound header anyways - so it's literally a single op per websocket.

All that said - I am more than happy to change anything you asked for.

I believe we end up with 3 options

2 options which change current behavior

We remove rack.upgrade.deflate and if the client wants deflate - the client gets deflate.

We switch the default for rack.upgrade.deflate to true (which allows for developer intervention if needed)

1 option which would not change current behavior

We leave the default as false and require developer buy in per connection

Yes, I totally agree with you on the importance of keeping exiting behavior the same (or, at least providing an easy way to revert to the existing default). Backwards compatibility is super important to me as well (and I really don't want to version bump to version 0.8.x just yet, as I am working on a new version for facil.io).

Anyway, I posted half my answer in a different thread by mistake... there's a global default value settable through the CLI (or during the method call to Iodine.listen). This will allow an easy opt-out using an environment variable or CLI command rather than a code change within the app.

This will allow a "smart" default that makes sense for most developers while both allowing developers to opt-out of the extension and allowing us to remove the rack.upgrade.deflate from the logic sequence and skip the SipHash and Hash Map operations required to retrieve the value.

wishdev · 2021-10-20T21:00:56Z

Just to sum things up - I'll get the global default setup - push a new PR for facil.io for its pieces - and I'll remove the overkill sections.

Thanks for the review. It may take a couple of days for me to organize things but I'll be back :)

boazsegev · 2021-10-20T21:24:41Z

Sounds great. Please note that the facil.io development branch for iodine is the 0.7.x branch (I plan to have iodine 0.8.x use the 0.8.x facil.io version, but I'm not trying to match version numbers or anything).

The master brunch and the 0.8.x branch are probably somewhat stale. I've been developing the core functionality at the facil.io org repos and I'm planning to move a lot of what I've done out of my personal account so the community can take over.

wishdev · 2021-10-23T20:05:11Z

The last commit should fix all the concerns. I'm also offering up #111 which I believe may be a better option and ends up with cleaner developer code. As I said there - both options work and are offered as alternatives.

wishdev added 2 commits October 18, 2021 21:48

Update rake version to allow Ruby 3.0+ to compile

84f4a8d

Fix Ruby 3.0 depreciation / Ruby 3.1 removal of rb_cData

46146c4

WebSocket permessage-deflate server implementation

1f67412

wishdev force-pushed the permessage-deflate-server branch from 9f2c3a8 to 1f67412 Compare October 19, 2021 22:36

boazsegev requested changes Oct 20, 2021

View reviewed changes

Update websocket deflate initialization

8333cec

boazsegev force-pushed the master branch from 282a101 to 95f9c01 Compare September 21, 2022 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

permessage-deflate server implementation #110

permessage-deflate server implementation #110

wishdev commented Oct 19, 2021

boazsegev commented Oct 19, 2021

boazsegev left a comment

boazsegev Oct 20, 2021

wishdev Oct 20, 2021

boazsegev Oct 20, 2021

boazsegev Oct 20, 2021

wishdev Oct 20, 2021

boazsegev Oct 20, 2021

boazsegev Oct 20, 2021

wishdev Oct 20, 2021

boazsegev Oct 20, 2021

wishdev Oct 20, 2021

boazsegev Oct 20, 2021 •

edited

wishdev commented Oct 20, 2021

boazsegev commented Oct 20, 2021

wishdev commented Oct 23, 2021

permessage-deflate server implementation #110

Are you sure you want to change the base?

permessage-deflate server implementation #110

Conversation

wishdev commented Oct 19, 2021

boazsegev commented Oct 19, 2021

boazsegev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boazsegev Oct 20, 2021 • edited

Choose a reason for hiding this comment

wishdev commented Oct 20, 2021

boazsegev commented Oct 20, 2021

wishdev commented Oct 23, 2021

boazsegev Oct 20, 2021 •

edited