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

When BodyHandler isn't the first handler, request may get stuck (not processed, client times out) #2438

Closed
guss77 opened this issue Jun 14, 2023 · 4 comments
Labels

Comments

@guss77
Copy link
Contributor

guss77 commented Jun 14, 2023

Version

4.4.3

Context

When setting up BodyHandler to process request bodies, if the BodyHandler is not the first handler (and the previous handlers take a bit of time to finish processing), then the BodyHandler.handle() method will not call RoutingContext.next() and will not pass processing to the next handler. As a result the request isn't being processed and the client will eventually time out and get an error.

I normally would like to do some processing on the headers before I commit to parse the body (reading authorization headers and looking them up in the database, nominally), or - if BodyHandler will fail (for example with Payload Too Large) I would still want to do some generic processing, such as logging the failed request using a logging handler, adding server-specific response headers, etc.

Do you have a reproducer?

This reproducer has a handler with a 50ms delay before calling the body handler, to simulate some IO.

Steps to reproduce

  1. Register a handler that runs before BodyHandler
  2. Have the first handler do some IO.

Extra

I'm not really clear if it is even possible for BodyHandler to work correctly if it isn't the first handler, but IMHO my points still stand:

  1. If BodyHandler gives up - it should fail the request with an appropriate exception, not just be like 🤷 and drop the request on the floor.
  2. There should be a way to do some processing before BodyHandler completes reading and parsing the body - if BodyHandler needs to register callbacks early or something, then it should be able to let it to do that but not block all the other handlers until all the request data has been read from the socket (which can take a long time and we might just want to close the socket early or sth).
@guss77 guss77 added the bug label Jun 14, 2023
@guss77
Copy link
Contributor Author

guss77 commented Jun 14, 2023

My repro is actually a bit broken - something about threads and contexts. In my actual workflow I'm using just Vert.x async processing where it repros quite well, but the minimal test case looks to be too minimal right now.

I'll fix it up in a bit, but the problem is definitely there.

@guss77
Copy link
Contributor Author

guss77 commented Jun 14, 2023

The repro is now properly showing the issue - it is running the same code twice: when setting the delay period to 0, the test passes, when setting the delay period to 10ms if gets stuck and times out.

@tsegismont tsegismont added invalid and removed bug labels Jun 19, 2023
@tsegismont
Copy link
Contributor

If you can't define the BodyHandler first, you must pause the request like this:

    router.post().handler(ctx -> {
      ctx.request().pause();
      System.out.println(Instant.now() + " Delaying before body processing");

Otherwise, all the content will be discarded before the BodyHandler gets invoked.

@guss77
Copy link
Contributor Author

guss77 commented Jun 19, 2023

@tsegismont - how about just handling the invalid situation with an error, instead of leaving the request unprocessed and unprocessable? Something like PR #2439 ?

Ideally there'll be a richer API, but if that cannot be the case - can we at least get an error?

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

No branches or pull requests

2 participants