extmod/modwebsocket: Add client support. #3406
Conversation
dd65bb8 to
f6bb8ff
Compare
This change adds websocket client support as per RFC6455. Section 5.1 states that a websocket client must mask all frames that it sends to the server. Further, section 5.3 states the masking key is a 32-bit value chosen at random by the client unique to each frame. These requirements are now implemented. A non-compliant debug mask is also supported, allowing control of the masking key.
f6bb8ff to
caec282
Compare
|
Thanks for this contribution! What's interesting and that I lately also was thinking how to "finish" this module and in particular add client support. But I thought also about other things, and it makes it not so easy... Let me however start with pointing few immediate problems with this patch as it is. |
| @@ -58,16 +63,39 @@ typedef struct _mp_obj_websocket_t { | |||
| STATIC mp_uint_t websocket_write(mp_obj_t self_in, const void *buf, mp_uint_t size, int *errcode); | |||
|
|
|||
| STATIC mp_obj_t websocket_make_new(const mp_obj_type_t *type, size_t n_args, size_t n_kw, const mp_obj_t *args) { | |||
| mp_arg_check_num(n_args, n_kw, 1, 2, false); | |||
| static const mp_arg_t allowed_args[] = { | |||
| { MP_QSTR_sock, MP_ARG_REQUIRED | MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL} }, | |||
| mp_arg_check_num(n_args, n_kw, 1, 2, false); | ||
| static const mp_arg_t allowed_args[] = { | ||
| { MP_QSTR_sock, MP_ARG_REQUIRED | MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL} }, | ||
| { MP_QSTR_use_blocking_writes, MP_ARG_BOOL, {.u_bool = false} }, |
There was a problem hiding this comment.
We always save for the space, so using such long arg names is no-no.
| static const mp_arg_t allowed_args[] = { | ||
| { MP_QSTR_sock, MP_ARG_REQUIRED | MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL} }, | ||
| { MP_QSTR_use_blocking_writes, MP_ARG_BOOL, {.u_bool = false} }, | ||
| { MP_QSTR_is_client, MP_ARG_BOOL, {.u_bool = false} }, |
| { MP_QSTR_sock, MP_ARG_REQUIRED | MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL} }, | ||
| { MP_QSTR_use_blocking_writes, MP_ARG_BOOL, {.u_bool = false} }, | ||
| { MP_QSTR_is_client, MP_ARG_BOOL, {.u_bool = false} }, | ||
| { MP_QSTR_debug_mask, MP_ARG_KW_ONLY | MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL} }, |
There was a problem hiding this comment.
I'm always for having debugging/diagnostic facilities in the code, but they should be properly #ifdef'ed and off by default.
| mp_obj_websocket_t *o = m_new_obj(mp_obj_websocket_t); | ||
| o->base.type = type; | ||
| o->sock = args[0]; | ||
| o->sock = arg_vals.sock.u_obj; | ||
| o->do_write_masking = !arg_vals.is_client.u_bool ? NO_WRITE_MASKING : NORMAL_WRITE_MASKING; |
There was a problem hiding this comment.
At least remove "!" and swap args. I'd also use normal "if" here.
| // masks will always be used. A seed could be derived from a | ||
| // network resource, a network interface's characteristics or | ||
| // statistics, or a platform specific resource. Examples of using | ||
| // a platform specific resource include reading an ESP8266's |
There was a problem hiding this comment.
I'm all for detailed comments, but specifics of a particular chip in a generic module are superfluous.
| // from a strong source of entropy. The "urandom" module doesn't | ||
| // qualify in this regard, but there isn't any cross-platform | ||
| // alternative. Fortunately, the purpose of masking is not | ||
| // cryptographically motivated. The "urandom" module should be |
There was a problem hiding this comment.
I'd remove such a statement, it's subjective.
| // 32-bit Random Number Generator register, or reading consecutive | ||
| // values from a floating analog pin. | ||
| < 8000 /td> | mp_obj_t dest[3]; | |
| mp_load_method(mp_module_get(MP_QSTR_urandom), MP_QSTR_getrandbits, dest); |
There was a problem hiding this comment.
The code to get a random value should be factored out to a function.
| if (self->do_write_masking == NO_WRITE_MASKING) { | ||
| out_sz = mp_stream_write_exactly(self->sock, buf, size, errcode); | ||
| } else { | ||
| byte masked_buf[size]; |
There was a problem hiding this comment.
Really? But size can be 1GB.
|
Besides the above the bigger conceptual problem is how to support non-blocking write mode for such stream wrappers: #3396 . I invite to think how the code you add can be made useful for async non-blocking writers. (If you don't have ideas/not interested, I can share some of my ideas.) |
|
Async implementation of websockets which needs client support: micropython/micropython-lib#228 |
|
@pfalcon Thanks for the quick review, feedback, and encouragement! I hope to get this wrapped up in short order and move on to further tasks. To provide context, I'm working on integrating with the AWS IoT MQTT-based service via websockets on an ESP8266. I have this working on the UNIX port, but need to do further work for the ESP8266 port due to the MCU's constraints. Ultimately this will require submitting more pull requests, and likely revisiting some stagnant pull requests from others. I think I am aware of the considerations related to writing code for an embedded environment, but I am finding translating this into specific decisions to be more challenging. Most of my experience is related to enterprise software, which has a completely different set of considerations. I will likely have some questions regarding your feedback shortly. Thanks for your guidance as I proceed! |
doc fix: remove the text about the non-existing clock specification
|
Closing due to inactivity (specifically, the feedback has not been addressed). This would be a great feature, so feel free to reopen against latest master. Things to consider would be how to make it work with uasyncio (ie non-blocking). |
This change adds websocket client support as per RFC6455. Section 5.1 states
that a websocket client must mask all frames that it sends to the server.
Further, section 5.3 states the masking key is a 32-bit value chosen at random
by the client unique to each frame. These requirements are now implemented.
A non-compliant debug mask is also supported, allowing control of the masking
key.