Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tag the sqe's to fix the fsm handling a double send cqe #104

Open
wants to merge 6 commits into
base: main-dev
Choose a base branch
from

Conversation

MarkReedZ
Copy link
Contributor

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.

sudo apt install wrk
wrk -t1 -c32 -d2s http://localhost:8545/ -s json.lua

@MarkReedZ
Copy link
Contributor Author

Also added a fix for never ending attempts to close already closed connections

   if (is_corrupted()) 
        if ( connection.stage != stage_t::waiting_to_close_k )
            return close_gracefully();

@ashvardanian
Copy link
Collaborator

Hi @MarkReedZ! Good catch!

Just a couple of clarification questions:

  1. Why are we using the top 4 bits? To store 4 states wee need only 2 bits. But it seems like you need 5 states (including zero), in that case you need only three state.
  2. How portable is pointer tagging on Linux?

One tiny request - can you please use clang-format for formatting? In VS Code I enable that extension by default on every save operation.

@MarkReedZ
Copy link
Contributor Author

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 }

@MarkReedZ
Copy link
Contributor Author

Formatting update and moved the tagging to the lower 2 bits.

@@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be std::size_t.

@@ -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)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The (std::uintptr_t)(&connection) may be a better choice in such places.

Comment on lines 92 to 95
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};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result of parsing error should be handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now return the same Invalid JSON error. We could add a new code "Invalid HTTP request" for the 2 errors here.

@ashvardanian
Copy link
Collaborator

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 git credentials may be ill-formed, as GitHub fails to link them to your account.

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.

@MarkReedZ
Copy link
Contributor Author

Going forward I'll follow the commit message rules and squash my commits on the branch prior to the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants