Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

permessage-deflate server implementation #110

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ext/iodine/http.h
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,7 @@ typedef struct {
void (*on_close)(intptr_t uuid, void *udata);
/** Opaque user data. */
void *udata;
int deflate;
} websocket_settings_s;

/**
Expand Down
4 changes: 4 additions & 0 deletions ext/iodine/http1.c
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
http_set_header(h, HTTP_HEADER_CONNECTION, fiobj_dup(HTTP_HVALUE_WS_UPGRADE));
http_set_header(h, HTTP_HEADER_UPGRADE, fiobj_dup(HTTP_HVALUE_WEBSOCKET));
http_set_header(h, HTTP_HEADER_WS_SEC_KEY, tmp);
Expand Down
6 changes: 6 additions & 0 deletions ext/iodine/http_internal.c
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ FIOBJ HTTP_HEADER_ORIGIN;
FIOBJ HTTP_HEADER_SET_COOKIE;
FIOBJ HTTP_HEADER_UPGRADE;
FIOBJ HTTP_HEADER_WS_SEC_CLIENT_KEY;
FIOBJ HTTP_HEADER_WS_SEC_EXTENSIONS;
FIOBJ HTTP_HEADER_WS_SEC_KEY;
FIOBJ HTTP_HVALUE_BYTES;
FIOBJ HTTP_HVALUE_CLOSE;
Expand All @@ -136,6 +137,7 @@ FIOBJ HTTP_HVALUE_MAX_AGE;
FIOBJ HTTP_HVALUE_NO_CACHE;
FIOBJ HTTP_HVALUE_SSE_MIME;
FIOBJ HTTP_HVALUE_WEBSOCKET;
FIOBJ HTTP_HVALUE_WS_DEFLATE;
FIOBJ HTTP_HVALUE_WS_SEC_VERSION;
FIOBJ HTTP_HVALUE_WS_UPGRADE;
FIOBJ HTTP_HVALUE_WS_VERSION;
Expand Down Expand Up @@ -172,6 +174,7 @@ static void http_lib_cleanup(void *ignr_) {
HTTPLIB_RESET(HTTP_HEADER_SET_COOKIE);
HTTPLIB_RESET(HTTP_HEADER_UPGRADE);
HTTPLIB_RESET(HTTP_HEADER_WS_SEC_CLIENT_KEY);
HTTPLIB_RESET(HTTP_HEADER_WS_SEC_EXTENSIONS);
HTTPLIB_RESET(HTTP_HEADER_WS_SEC_KEY);
HTTPLIB_RESET(HTTP_HVALUE_BYTES);
HTTPLIB_RESET(HTTP_HVALUE_CLOSE);
Expand All @@ -182,6 +185,7 @@ static void http_lib_cleanup(void *ignr_) {
HTTPLIB_RESET(HTTP_HVALUE_NO_CACHE);
HTTPLIB_RESET(HTTP_HVALUE_SSE_MIME);
HTTPLIB_RESET(HTTP_HVALUE_WEBSOCKET);
HTTPLIB_RESET(HTTP_HVALUE_WS_DEFLATE);
HTTPLIB_RESET(HTTP_HVALUE_WS_SEC_VERSION);
HTTPLIB_RESET(HTTP_HVALUE_WS_UPGRADE);
HTTPLIB_RESET(HTTP_HVALUE_WS_VERSION);
Expand Down Expand Up @@ -213,6 +217,7 @@ static void http_lib_init(void *ignr_) {
HTTP_HEADER_SET_COOKIE = fiobj_str_new("set-cookie", 10);
HTTP_HEADER_UPGRADE = fiobj_str_new("upgrade", 7);
HTTP_HEADER_WS_SEC_CLIENT_KEY = fiobj_str_new("sec-websocket-key", 17);
HTTP_HEADER_WS_SEC_EXTENSIONS = fiobj_str_new("sec-websocket-extensions", 24);
HTTP_HEADER_WS_SEC_KEY = fiobj_str_new("sec-websocket-accept", 20);
HTTP_HVALUE_BYTES = fiobj_str_new("bytes", 5);
HTTP_HVALUE_CLOSE = fiobj_str_new("close", 5);
Expand All @@ -224,6 +229,7 @@ static void http_lib_init(void *ignr_) {
HTTP_HVALUE_NO_CACHE = fiobj_str_new("no-cache, max-age=0", 19);
HTTP_HVALUE_SSE_MIME = fiobj_str_new("text/event-stream", 17);
HTTP_HVALUE_WEBSOCKET = fiobj_str_new("websocket", 9);
HTTP_HVALUE_WS_DEFLATE = fiobj_str_new("permessage-deflate", 18);
HTTP_HVALUE_WS_SEC_VERSION = fiobj_str_new("sec-websocket-version", 21);
HTTP_HVALUE_WS_UPGRADE = fiobj_str_new("Upgrade", 7);
HTTP_HVALUE_WS_VERSION = fiobj_str_new("13", 2);
Expand Down
2 changes: 2 additions & 0 deletions ext/iodine/http_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ Constants that shouldn't be accessed by the users (`fiobj_dup` required).

extern FIOBJ HTTP_HEADER_ACCEPT_RANGES;
extern FIOBJ HTTP_HEADER_WS_SEC_CLIENT_KEY;
extern FIOBJ HTTP_HEADER_WS_SEC_EXTENSIONS;
extern FIOBJ HTTP_HEADER_WS_SEC_KEY;
extern FIOBJ HTTP_HVALUE_BYTES;
extern FIOBJ HTTP_HVALUE_CLOSE;
Expand All @@ -82,6 +83,7 @@ extern FIOBJ HTTP_HVALUE_MAX_AGE;
extern FIOBJ HTTP_HVALUE_NO_CACHE;
extern FIOBJ HTTP_HVALUE_SSE_MIME;
extern FIOBJ HTTP_HVALUE_WEBSOCKET;
extern FIOBJ HTTP_HVALUE_WS_DEFLATE;
extern FIOBJ HTTP_HVALUE_WS_SEC_VERSION;
extern FIOBJ HTTP_HVALUE_WS_UPGRADE;
extern FIOBJ HTTP_HVALUE_WS_VERSION;
Expand Down
38 changes: 32 additions & 6 deletions ext/iodine/iodine_connection.c
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,27 @@ Ruby Connection Methods - write, close open? pending
* Use {pending} to test how many `write` operations are pending completion
* (`on_drained(client)` will be called when they complete).
*/
static VALUE iodine_connection_write(VALUE self, VALUE data) {
static VALUE iodine_connection_write(int argc, VALUE* argv, VALUE self) {
VALUE data, opts;

static ID keyword_ids[1];
VALUE kwargs[1];
VALUE deflate = Qnil;

ws_s *socket;
int rsv = 0;

if (!keyword_ids[0]) {
CONST_ID(keyword_ids[0], "deflate");
}

rb_scan_args(argc, argv, "1:", &data, &opts);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (!NIL_P(opts)) {
rb_get_kwargs(opts, keyword_ids, 0, 1, kwargs);
if (kwargs[0] != Qundef) { deflate = kwargs[0]; }
}

iodine_connection_data_s *c = iodine_connection_validate_data(self);
if (!c || fio_is_closed(c->info.uuid)) {
// don't throw exceptions - closed connections are unavoidable.
Expand All @@ -187,8 +207,14 @@ static VALUE iodine_connection_write(VALUE self, VALUE data) {
switch (c->info.type) {
case IODINE_CONNECTION_WEBSOCKET:
/* WebSockets*/
websocket_write(c->info.arg, IODINE_RSTRINFO(data),
rb_enc_get(data) == IodineUTF8Encoding);
socket = c->info.arg;

if (websocket_has_deflator(socket) && deflate) {
rsv = 4;
}

websocket_write(socket, IODINE_RSTRINFO(data),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

rb_enc_get(data) == IodineUTF8Encoding, rsv);
return Qtrue;
break;
case IODINE_CONNECTION_SSE:
Expand Down Expand Up @@ -437,7 +463,7 @@ static void iodine_on_pubsub(fio_msg_s *msg) {
fiobj_send_free(data->info.uuid, fiobj_dup(s));
} else {
fwrite("-", 1, 1, stderr);
websocket_write(data->info.arg, msg->msg, (block == Qnil));
websocket_write(data->info.arg, msg->msg, (block == Qnil), 0);
}
return;
}
Expand Down Expand Up @@ -900,9 +926,9 @@ void iodine_connection_init(void) {
IodineStore.add(RAWSymbol);

// define the Connection Class and it's methods
ConnectionKlass = rb_define_class_under(IodineModule, "Connection", rb_cData);
ConnectionKlass = rb_define_class_under(IodineModule, "Connection", rb_cObject);
rb_define_alloc_func(ConnectionKlass, iodine_connection_data_alloc_c);
rb_define_method(ConnectionKlass, "write", iodine_connection_write, 1);
rb_define_method(ConnectionKlass, "write", iodine_connection_write, -1);
rb_define_method(ConnectionKlass, "close", iodine_connection_close, 0);
rb_define_method(ConnectionKlass, "open?", iodine_connection_is_open, 0);
rb_define_method(ConnectionKlass, "pending", iodine_connection_pending, 0);
Expand Down
20 changes: 19 additions & 1 deletion ext/iodine/iodine_http.c
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ VALUE IODINE_R_HIJACK;
VALUE IODINE_R_HIJACK_IO;
VALUE IODINE_R_HIJACK_CB;

static VALUE RACK_WS_EXTENSIONS;
static VALUE RACK_UPGRADE;
static VALUE RACK_UPGRADE_DEFLATE;
static VALUE RACK_UPGRADE_Q;
static VALUE RACK_UPGRADE_SSE;
static VALUE RACK_UPGRADE_WEBSOCKET;
Expand Down Expand Up @@ -199,10 +201,24 @@ static void iodine_ws_attach(http_s *h, VALUE handler, VALUE env) {
if (io == Qnil)
return;

int deflate = 0;

// check if permessage-deflate allowed
// must have header from client for extensions
// must have the permessage-deflate extension requested
// must have server authorize deflation
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) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. We have the inbound header
  2. The inbound header contains permessage-deflate
  3. 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

  1. We remove rack.upgrade.deflate and if the client wants deflate - the client gets deflate.
  2. 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

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

Copy link
Owner

@boazsegev boazsegev Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

deflate = 1;
}

http_upgrade2ws(h, .on_message = iodine_ws_on_message,
.on_open = iodine_ws_on_open, .on_ready = iodine_ws_on_ready,
.on_shutdown = iodine_ws_on_shutdown,
.on_close = iodine_ws_on_close, .udata = (void *)io);
.on_close = iodine_ws_on_close, .udata = (void *)io,
.deflate = deflate);
}

/* *****************************************************************************
Expand Down Expand Up @@ -1120,7 +1136,9 @@ void iodine_init_http(void) {
rack_set(IODINE_R_HIJACK, "rack.hijack");
rack_set(IODINE_R_HIJACK_CB, "iodine.hijack_cb");

rack_set(RACK_WS_EXTENSIONS, "HTTP_SEC_WEBSOCKET_EXTENSIONS");
rack_set(RACK_UPGRADE, "rack.upgrade");
rack_set(RACK_UPGRADE_DEFLATE, "rack.upgrade.deflate");
rack_set(RACK_UPGRADE_Q, "rack.upgrade?");
rack_set_sym(RACK_UPGRADE_SSE, "sse");
rack_set_sym(RACK_UPGRADE_WEBSOCKET, "websocket");
Expand Down
2 changes: 1 addition & 1 deletion ext/iodine/iodine_mustache.c
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,7 @@ void iodine_init_mustache(void) {
rb_global_variable(&filename_id);
rb_global_variable(&data_id);
rb_global_variable(&template_id);
VALUE tmp = rb_define_class_under(IodineModule, "Mustache", rb_cData);
VALUE tmp = rb_define_class_under(IodineModule, "Mustache", rb_cObject);
rb_define_alloc_func(tmp, iodine_mustache_data_alloc_c);
rb_define_method(tmp, "initialize", iodine_mustache_new, -1);
rb_define_method(tmp, "render", iodine_mustache_render, 1);
Expand Down
2 changes: 1 addition & 1 deletion ext/iodine/iodine_store.c
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ struct IodineStorage_s IodineStore = {
void iodine_storage_init(void) {
fio_store_capa_require(&iodine_storage, 512);
VALUE tmp =
rb_define_class_under(rb_cObject, "IodineObjectStorage", rb_cData);
rb_define_class_under(rb_cObject, "IodineObjectStorage", rb_cObject);
VALUE storage_obj =
TypedData_Wrap_Struct(tmp, &storage_type_struct, &iodine_storage);
// rb_global_variable(&iodine_storage_obj);
Expand Down
2 changes: 1 addition & 1 deletion ext/iodine/iodine_tls.c
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ void iodine_init_tls(void) {
IODINE_MAKE_SYM(private_key);
IODINE_MAKE_SYM(password);

IodineTLSClass = rb_define_class_under(IodineModule, "TLS", rb_cData);
IodineTLSClass = rb_define_class_under(IodineModule, "TLS", rb_cObject);
rb_define_alloc_func(IodineTLSClass, iodine_tls_data_alloc_c);
rb_define_method(IodineTLSClass, "initialize", iodine_tls_new, -1);
rb_define_method(IodineTLSClass, "use_certificate",
Expand Down
94 changes: 94 additions & 0 deletions ext/iodine/websocket_deflate.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/*
copyright: Boaz Segev, 2017-2019
license: MIT

Feel free to copy, use and enjoy according to the license specified.
*/
#ifndef H_WEBSOCKET_DEFLATE_H
/**\file

A single file WebSocket permessage-deflate wrapper

*/
#define H_WEBSOCKET_DEFLATE_H
#include <fiobj.h>
#include <stdlib.h>
#include <zlib.h>

#define WS_CHUNK 16384

z_stream *new_z_stream() {
z_stream *strm = malloc(sizeof(*strm));

*strm = (z_stream){
.zalloc = Z_NULL,
.zfree = Z_NULL,
.opaque = Z_NULL,
};

return strm;
}

z_stream *new_deflator() {
z_stream *strm = new_z_stream();

int ret = deflateInit2(strm, Z_DEFAULT_COMPRESSION, Z_DEFLATED,
-MAX_WBITS, 4, Z_DEFAULT_STRATEGY);

return strm;
}

z_stream *new_inflator() {
z_stream *strm = new_z_stream();

inflateInit2(strm, -MAX_WBITS);

return strm;
}

int deflate_message(fio_str_info_s src, FIOBJ dest, z_stream *strm) {
int ret, flush;
unsigned have;
unsigned char out[WS_CHUNK];

strm->avail_in = src.len;
strm->next_in = src.data;

do {
strm->avail_out = WS_CHUNK;
strm->next_out = out;
ret = deflate(strm, Z_SYNC_FLUSH);
have = WS_CHUNK - strm->avail_out;
fiobj_str_write(dest, out, have);
} while (strm->avail_out == 0);

return Z_OK;
}

int inflate_message(fio_str_info_s src, FIOBJ dest, z_stream *strm) {
int ret;
unsigned have;
unsigned char out[WS_CHUNK];

strm->avail_in = src.len;
strm->next_in = src.data;

do {
strm->avail_out = WS_CHUNK;
strm->next_out = out;
ret = inflate(strm, Z_SYNC_FLUSH);
switch (ret) {
case Z_NEED_DICT:
ret = Z_DATA_ERROR;
case Z_DATA_ERROR:
case Z_MEM_ERROR:
(void)inflateEnd(strm);
return ret;
}
have = WS_CHUNK - strm->avail_out;
fiobj_str_write(dest, out, have);
} while (strm->avail_out == 0);

return ret;
}
#endif
Loading