-
Notifications
You must be signed in to change notification settings - Fork 51
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
84f4a8d
46146c4
1f67412
8333cec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please automate the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
@@ -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; | ||
} | ||
|
@@ -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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is better to remove the 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Then the if statement make 3 checks
So there is a check for the inbound header and it is here. As far as The default could be changed to true very easily as a simple alternative as well. I'm not certain that leaving something like 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 option which would not change current behavior
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 |
||
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); | ||
} | ||
|
||
/* ***************************************************************************** | ||
|
@@ -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"); | ||
|
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 |
There was a problem hiding this comment.
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 thepermessage-deflate
value in theSec-WebSocket-Extensions
header.Web app developers shouldn't have to know anything about the
permessage-deflate
WebSocket extension. The choice to enable or disabledeflate
support should be limited to the settings where the value0
is reserved to be replaced with a "smart" default and the "disabled" value should be -1.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 theIodine.listen
method which sets the value in theiodine_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.