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

Multipart request hangs if server throws an error (ending it prematurely) #22

Closed
nemphys opened this issue Jul 2, 2020 · 6 comments
Closed

Comments

@nemphys
Copy link

nemphys commented Jul 2, 2020

Hi, I have been facing this issue lately (which was really hard to debug), when a .join() call to a CompletableFuture of a multipart upload request (called in a background thread) would hang until the request timeout was exceeded.

After some extensive testing, I realised that this happens when the server throws an error at some point (either at the very beginning or during the files upload) and sends a 500 response. When this happens, the CompletableFuture does not complete (until the timeout is hit).

I am fairly certain that this is not a bug of Methanol, since it also happens with a custom MultiPartBodyPublisher I used before I switched to Methanol's implementation. Also, the server side seems to handle the response correctly, since testing the same request with Postman immediately produces the desired result (the request is ended with an error response).

Nevertheless, I am wondering if you have any idea of how to handle this properly.

@mizosoft
Copy link
Owner

mizosoft commented Jul 4, 2020

Hi @nemphys, I'm not sure I'm familiar with this problem. I've tried to simulate this scenario with com.sun.net.httpserver.HttpServer (read some bytes then close and send 500) but the response completes normally. After making sure you're using the latest JDK, try to replicate this with a mockserver to see if the problem still persists, but be aware that the problem might be from the server itself when the client performs retries (see this).

@nemphys
Copy link
Author

nemphys commented Jul 4, 2020

Actually, the server is a Node.js 12.x instance running express. I don't think it is retry-related, since I only see 1 request on the server side.
After a little digging, I think the issue is that HTTPClient first tries to finish with the request sending and only then starts receiving the response; could be wrong, though, since the code is a little too complicated (I think I spotted this somewhere inside the Exchange class).

@mizosoft
Copy link
Owner

mizosoft commented Jul 4, 2020

I've skimmed through Exchange and I believe it does try to finish sending the request body before reading the response. I'm not exactly sure if that's a problem since the server will probably close the connection (completing the request exceptionally) or read and discard the request if it has problems.

With expect-continue however, the client reads response headers before proceeding with the request body (maybe try the request with expect-continue to see what happens?).

I'd try the same scenario using another async HTTP client to see if it handles this correctly. If so, maybe it's a good idea to post the problem to net-dev.

@mizosoft
Copy link
Owner

Hey @nemphys any updates on this?

@nemphys
Copy link
Author

nemphys commented Jul 17, 2020

Sorry, I didn't realise you were expecting something from me, since we established that the response starts to be read after the request is finished being sent. I thought this was a deal breaker and decided to live with it until something changes in HTTPClient.

@mizosoft
Copy link
Owner

I see...
I'll close this then since it appears to have nothing to do with Methanol.

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

No branches or pull requests

2 participants