metarhia / jstp

Fast RPC for browser and Node.js based on TCP, WebSocket, and MDSF
https://metarhia.github.io/jstp
Other
142 stars 10 forks source link

ws,server: change max frame and message size #383

Closed belochub closed 5 years ago

belochub commented 5 years ago

Change the maximally allowed frame and message size for WS server to a maximal size of JSTP message allowed (8 MiB).

This fixes the same issue as https://github.com/metarhia/jstp/pull/381 with the least amount of changes, thus I'd prefer to land this PR instead of https://github.com/metarhia/jstp/pull/381.

lundibundi commented 5 years ago

Wouldn't it be possible to actually send messages (from client) of size more than 8MB with #381 (once we make MAX_MESSAGE_SIZE on the server configurable)? While this change would still require a user to manually split their messages. IMO we should land both of those PRs IIUC.

belochub commented 5 years ago

@lundibundi It is possible to send messages of size more than 8MB after this PR lands because the limit is set to 8MiB. But regardless of whether we land the https://github.com/metarhia/jstp/pull/381 or not, it will be possible for the client to send messages bigger than 8MiB if we make the MAX_MESSAGE_SIZE configurable, there still will be no need to split the messages on the client side.

aqrln commented 5 years ago

@lundibundi there's some context missing in #381 because at some point I stopped reviewing it on GitHub and brought it up in person with @belochub. Browsers don't limit message size to 64 KiB or whatever, the purpose of bufferedAmount it to allow you to implement rate limiting (and if you check its value for anything else than to compare it with 0, then how the value changes over time is what is important, not a momentary value), and the only reason connections were dropped is because the server dropped them due to its default settings.

lundibundi commented 5 years ago

@aqrln thanks for the explanation. LGTM then.

belochub commented 5 years ago

Landed in https://github.com/metarhia/jstp/commit/c008830ffdff7d96fbae3f9ff513de3960d42aa7.