Tag the sqe's to fix the fsm handling a double send cqe #104
Conversation
|
Also added a fix for never ending attempts to close already closed connections |
|
Hi @MarkReedZ! Good catch! Just a couple of clarification questions:
One tiny request - can you please use |
|
I'm looking for alternatives to the pointer tagging and I had a hack disabling auto formatting on save with VS code. I'll fix the formatting on my next commit. The latest commit fixes another issue. I've added a new partial request test that does a full request followed by a partial. The code was passing { body.data(), input_size } to the parser instead of { body.data(), content_length } |
|
Formatting update and moved the tagging to the lower 2 bits. |
src/helpers/parse.hpp
Outdated
| @@ -117,6 +119,7 @@ struct parsed_request_t { | |||
| std::string_view content_type{}; | |||
| std::string_view content_length{}; | |||
| std::string_view body{}; | |||
| int json_length; | |||
There was a problem hiding this comment.
This should be std::size_t.
src/engine_uring.cpp
Outdated
| @@ -790,7 +800,7 @@ bool engine_t::consider_accepting_new_connection() noexcept { | |||
| uring_sqe = io_uring_get_sqe(&uring); | |||
| io_uring_prep_accept_direct(uring_sqe, socket, &connection.client_address, &connection.client_address_len, 0, | |||
| IORING_FILE_INDEX_ALLOC); | |||
| io_uring_sqe_set_data(uring_sqe, &connection); | |||
| io_uring_sqe_set_data(uring_sqe, (void*)(uring_acpt_tag_k | uint64_t(&connection))); | |||
There was a problem hiding this comment.
The (std::uintptr_t)(&connection) may be a better choice in such places.
src/engine_uring.cpp
Outdated
| static constexpr std::size_t uring_acpt_tag_k{0}; | ||
| static constexpr std::size_t uring_recv_tag_k{1}; | ||
| static constexpr std::size_t uring_send_tag_k{2}; | ||
| static constexpr std::size_t uring_stat_tag_k{3}; |
There was a problem hiding this comment.
Maybe better to use an enum class using_verb_t : std::uintptr_t {}, and later reuse that enum-type instead of uint64_t type in stage_t and mutex_t. Would make things more readable.
| @@ -166,8 +169,11 @@ inline std::variant<parsed_request_t, default_error_t> split_body_headers(std::s | |||
| if (pos == std::string_view::npos) | |||
| return default_error_t{-32700, "Invalid JSON was received by the server."}; | |||
| req.body = body.substr(pos + 4); | |||
| } else | |||
| auto res = std::from_chars(req.content_length.begin(), req.content_length.end(), req.json_length); | |||
There was a problem hiding this comment.
The result of parsing error should be handled.
There was a problem hiding this comment.
We now return the same Invalid JSON error. We could add a new code "Invalid HTTP request" for the 2 errors here.
|
Hi @MarkReedZ! Thanks for your contributions, very useful! I've added a few notes on things that catch my eye. Let me know if you can polish those! PS1: I notice, that your PS2: I traditionally squash your commits, renaming them to match the scheme recognized by the semantic versioning engine. If you name and structure the commit messages similar to the rest, it will be easier for me to preserve all of your work in the original shape. |
|
Going forward I'll follow the commit message rules and squash my commits on the branch prior to the PR. |
io_uring_prep_send can return multiple completions for partial data. However it can also return a 0 completion after a full completion which trips up the state machine as we move to expecting_reception after all the data is written.
The fix is to tag the user_data with the request type and ignore send done completions when in expecting_reception.
To see the bug use wrk which parses the responses and will hang when the bug occurs.