esp-rs / esp-idf-svc

Type-Safe Rust Wrappers for various ESP-IDF services (WiFi, Network, Httpd, Logging, etc.)
https://docs.esp-rs.org/esp-idf-svc/
Apache License 2.0
324 stars 179 forks source link

Session context for websockets #266

Closed jordanhalase closed 1 year ago

jordanhalase commented 1 year ago

I found the websocket code for the HTTP server at esp_idf_svc::http::server::ws, but am having a little trouble figuring out how to add the session context to this.

When writing code in C I'm used to setting sess_ctx in httpd_req_t to some malloc()'d struct on open (and maybe safely mutating some global data) and setting the free_ctx member to free() the data (and maybe safely mutating some global data).

I'm not sure how to achieve this with esp-idf-svc. Is there currently a way to have custom websocket session data or is it to be implemented? I need websocket session contexts to send some metadata/setup information before continuing.

Thanks

ivmarkov commented 1 year ago

Usually you don't need this equilibristics with Rust, as the closures in Rust (unlike regular C functions) can close over any data they want to use.

jordanhalase commented 1 year ago

Usually you don't need this equilibristics with Rust, as the closures in Rust (unlike regular C functions) can close over any data they want to use.

The argument to EspHttpServer::ws_handler() is a Fn() though, not an FnMut(), but also, the context should be allocated per-session, but the closure passed only captures already existing data at registration. user_ctx and sess_ctx are different things. sess_ctx is meant to get allocated and deallocated for every websocket open and close.

How would I implement a WebSocket that counts words given to it? So if I pass it "hello world" it replies with "2", and then pass it "I like Rust" it replies with "5"? And whenever a WebSocket is opened by anybody it should start from some default state and is unique to each user.

In C I do this in the handler by setting httpd_req_t::sess_ctx to some custom malloc'd struct if it's NULL, and set httpd_req_t::free_ctx to my free func.

In Rust I can do this with unsafe code and setting sess_ctx to Box::into_raw(Box::new(MyStruct::new())) if NULL then setting free_ctx to an extern "C" function that calls Box::from_raw() to free it when the WebSocket closes.

ivmarkov commented 1 year ago

Usually you don't need this equilibristics with Rust, as the closures in Rust (unlike regular C functions) can close over any data they want to use.

The argument to EspHttpServer::ws_handler() is a Fn() though, not an FnMut(), but also, the context should be allocated per-session, but the closure passed only captures already existing data at registration. user_ctx and sess_ctx are different things. sess_ctx is meant to get allocated and deallocated for every websocket open and close.

The difference between Fn and FnMut is the difference between a function that is shared across multiple threads (and so it needs to Arc<Mutex<>> any shared state it uses), versus a function that is dedicated to a single thread (and hence it can safely mutate its data).

While the ESP IDF HTTPD server happens to use a lousy single threaded (yet blocking!) implementation, the generic HTTPD Handler signature in embedded-svc does not know that and follows the Fn pattern rather than FnMut. While we can do a custom Handler trait for esp-idf-svc specifically, I don't think it is worth the effort and in general I see the ESP IDF HTTPD only as a stop-gap solution until somebody finally helps me fix the HTTPD+WS server in edge-net.

How would I implement a WebSocket that counts words given to it? So if I pass it "hello world" it replies with "2", and then pass it "I like Rust" it replies with "5"? And whenever a WebSocket is opened by anybody it should start from some default state and is unique to each user.

Did you notice EspHttpWsconnection::is_new()? It tells you whether this is a connection for a new websocket connection. So if it is true (and it will be true only once), you should create a new Arc<Mutex<>> state for it. And keep it in some sort of vec or whatever. And drop it when no longer necessary. And you might also optionally use HttpWsconnection::session() as an ID of the connection.

In C I do this in the handler by setting httpd_req_t::sess_ctx to some custom malloc'd struct if it's NULL, and set httpd_req_t::free_ctx to my free func.

In Rust I can do this with unsafe code and setting sess_ctx to Box::into_raw(Box::new(MyStruct::new())) if NULL then setting free_ctx to an extern "C" function that calls Box::from_raw() to free it when the WebSocket closes.

Sure, I get it. I just want to make really sure, that we need C style idioms (or some aspects of it) before implementing any of it.

jordanhalase commented 1 year ago

While the ESP IDF HTTPD server happens to use a lousy single threaded (yet blocking!) implementation, the generic HTTPD Handler signature in embedded-svc does not know that and follows the Fn pattern rather than FnMut. While we can do a custom Handler trait for esp-idf-svc specifically, I don't think it is worth the effort and in general I see the ESP IDF HTTPD only as a stop-gap solution until somebody finally helps me fix the HTTPD+WS server in edge-net.

Yeah I saw some of the tricks esp-idf-svc has to pull to cope with the limitations of esp_httpd. I also see it as a stopgap although async Rust is still less mature than IDF. I just wanted to make sure there was no solution before I started working on one. I've thought about adding a custom trait to deal with this.

I'll definitely look into edge-net though.

Did you notice EspHttpWsconnection::is_new()? It tells you whether this is a connection for a new websocket connection. So if it is true (and it will be true only once), you should create a new Arc<Mutex<>> state for it. And keep it in some sort of vec or whatever. And drop it when no longer necessary. And you might also optionally use HttpWsconnection::session() as an ID of the connection.

I thought about that, but I had some trouble finding out how I could drop it when the websocket is closed. Maybe it's possible I didn't look hard enough.

Sure, I get it. I just want to make really sure, that we need C style idioms (or some aspects of it) before implementing any of it.

In the meantime, perhaps I could add a pub unsafe raw_handler() to EspHttpServer. It wouldn't close this issue entirely, but would be a good-enough stopgap solution for registering raw C handlers wherever the Rust API falls short.

ivmarkov commented 1 year ago

I would rather say... how about adding an option to the existing handler to be called with an EspHttpWsConnection which is in EspHttpWsConnection::Closed state? This code is almost there, it is only that your handler is not called when the connection is closed. This would allow you to close the gap without pub unsafe and friends?

ivmarkov commented 1 year ago

As for async Rust being less mature than IDF... it is not an "either" / "or". You can use async Rust with ESP IDF. In fact, you can even use async Rust with ESP IDF HTTPD. It is just that you'll get the same lousy single-threaded behavior. As in once request/response has to be handled completely before the next one can kick in. The server - being single-threaded and blocking - cannot interleave the processing of multiple HTTP requests "in parallel".

jordanhalase commented 1 year ago

I would rather say... how about adding an option to the existing handler to be called with an EspHttpWsConnection which is in EspHttpWsConnection::Closed state? This code is almost there, it is only that your handler is not called when the connection is closed. This would allow you to close the gap without pub unsafe and friends?

That would definitely be easier to implement (albeit less wieldy to the user) than having some EspHttpWsSession trait. As long as an Arc<Mutex<T>> can be accessed and updated from an Fn() closure, I see no reason why this couldn't work.

As far as async immaturity goes I mainly meant that embassy doesn't feel all the way there yet. It's a nervous idea to use github-only crates in production. Of course that'll change eventually.

ivmarkov commented 1 year ago

Most of these, if not all, are not github-only since an year or so. And almost all of them except the HAL crates in fact work just fine with esp idf.

jordanhalase commented 1 year ago

Is a WebSocket frame type of HTTPD_WS_TYPE_CLOSE guaranteed to be sent to a WebSocket handler when a connection is closed?

The code that currently only sends the two variants sends EspHttpWsConnection::New when httpd_req_t::method is HTTP_GET. It sends EspHttpWsConnection::Receiving otherwise.

If it sends any close message at all, I think it would be done as a WebSocket frame, but I don't know if that's guaranteed.

ivmarkov commented 1 year ago

There is a close handler in the code that gets called when the underlying tcp/ip socket is closed. Otherwise how do you think the C code knows when the underlying structure is no longer used and can be free-d?

jordanhalase commented 1 year ago

I keep forgetting the bindings are overriding the close handlers. I believe I got it figured out now, I should write some example code and make a PR.

On a similar issue, I discovered that calling ws.recv(&mut []); to obtain the frame size followed by a regular ws.recv(&mut buf) causes a panic. httpd_ws: httpd_ws_recv_frame: WS frame is not properly masked. and thread '<unnamed>' panicked at 'Unknown frame type: 4', esp-idf-svc-0.46.0\src\http\server.rs:1032:22. I think httpd_ws_recv_frame() mutates the websocket...even if you call it with a zero-frame to get size data...fun... So it only works properly if you call that function with a zeroed frame exactly once, then immediately re-use that frame to actually grab the data.

ivmarkov commented 1 year ago

On a similar issue, I discovered that calling ws.recv(&mut []); to obtain the frame size followed by a regular ws.recv(&mut buf) causes a panic. httpd_ws: httpd_ws_recv_frame: WS frame is not properly masked. and thread '<unnamed>' panicked at 'Unknown frame type: 4', esp-idf-svc-0.46.0\src\http\server.rs:1032:22. I think httpd_ws_recv_frame() mutates the websocket...even if you call it with a zero-frame to get size data...fun... So it only works properly if you call that function with a zeroed frame exactly once, then immediately re-use that frame to actually grab the data.

You are right. I'm swamped with other work, but I pushed a potential fix. If you can patch with master and then let me know if it now works for you, that would be great!