fix(ws server): batch wait until all methods has been executed.
#542
Conversation
ws-server/src/server.rs
Outdated
| let futs = batch | ||
| .into_iter() | ||
| .filter_map(|req| m.execute_with_resources(&tx_batch, req, conn_id, r)); | ||
| futures_util::future::join_all(futs).await; |
There was a problem hiding this comment.
It's a bit funny to read the responses off a different future in collect_batch_response. Is it possible to add that result-reading task to futs and join_all on that?
There was a problem hiding this comment.
true, we just need to close the channel after the method calls futures has completed.
otherwise, collect_batch_response won't complete.
Methods::execute_with_resource -> BoxFuture<()> thus only possible to read result via another future/channel i.e, not directly from the result from the future without any major refactoring....
ws-server/src/server.rs
Outdated
| let r = &resources; | ||
| let m = &methods; |
There was a problem hiding this comment.
These re-borrows are a bit awkward, not sure if there's a way around that though (impl AsRef for Methods?). :/
There was a problem hiding this comment.
lemme check if I can remove the async move below otherwise it's required :(
ws-server/src/server.rs
Outdated
| if !batch.is_empty() { | ||
| let futs = batch | ||
| .into_iter() | ||
| .filter_map(|req| m.execute_with_resources(&tx_batch, req, conn_id, r)); |
There was a problem hiding this comment.
Does this mean that responses could come back in a different order to the request order in the batch?
eg if we have a batch request to first slow_hello and then say_hello, and then collect those up via a channel, we'll get the say_hello response back first.
Do we care that the response order might not match the request order (if I'm even right about this!)?
There was a problem hiding this comment.
good question, yes responses could be in different order but it's intended because the spec doesn't require responses to be in order 👇
The Response objects being returned from a batch call MAY be returned in any order within the Array. The Client SHOULD match contexts between the set of Request objects and the resulting set of Response objects based on the id member within each Object.
I should have put a comment in the code about this but it's kind of an optimization :)
This fixes a bug in the ws-server when any
async methodsare included in batch, the futures were not awaited on that "may" cause method calls to be ignored. In worst case just send back]