Bug with 204 HTTP response

[AuHau]

I encountered a very strange behavior when I created a PATCH endpoint with 204 response. Requesting this endpoint using httpclient.HttpClient works fine. However, if I then make a subsequent GET request to a different endpoint, that request will fail with Connection was closed before the full request has been made [ProtocolError].

I am unsure where the problem lies; it could be in the HttpClient or in the Chrono server implementation. I have already spent too much time on this. Therefore, I am sharing it here and changing the response type from 204 to 200 as workaround.

I have created a reproduction repository for anyone interested in investigating further: https://github.com/AuHau/nim-http204-error

[cheatfate]

Its not problem of chronos server implementation, and not a presto problem too.
By default chronos server do not support HTTP pipelining, and so it always responds with Connection: close, so it indicates that server is not going to keep this connection opened and will going to close it.
This is what presto responds to your first /ping request:

HTTP/1.1 200 OK
Server: nim-presto/0.0.3 (amd64/windows)
Content-Length: 4
Content-Type: text/plain
Date: Wed, 21 Feb 2024 16:47:15 GMT
Connection: close

pong

After that you creating new HttpClient instance which will establish new connection to the presto server and this is presto server response:

HTTP/1.1 204 No Content
Server: nim-presto/0.0.3 (amd64/windows)
Content-Length: 0
Date: Wed, 21 Feb 2024 16:47:15 GMT
Connection: close

As you can see Connection: close is in place, so presto will not keep this connection online and will going to close it.

As you can see on this image, FIN packet being set indicating that presto wants to close this connection. But code reuses same connection and sends new request to the presto which it acknowledges but still repeats sending FIN packet to the client and so you have received error you see.

After long time working with HTTP pipelining mode, fixing bugs and helping people in such situation like this one, we decided to change defaults and did not support HTTP pipelining by default. So here 2 ways you can select.

1. Enable presto HTTP pipelining.
2. Use single HttpClient instance for single request.

I would better use 2 instead of 1, because stdlib's HttpClient is not very accurate on detecting HTTP conditions. As you can see in this example.

[cheatfate]

And thank you for your reproduction repo it helps a lot.

[AuHau]

I am not sure if I am convinced by your reasoning. Why then, does everything work as expected if the response of the PATCH endpoint is 200? The Connection: close should be present there as well...

[cheatfate]

It happens because your first test
https://github.com/AuHau/nim-http204-error/blob/master/test.nim#L6-L10
performs only one request to server, but your second test performs 2 requests using same instance of HttpClient.
https://github.com/AuHau/nim-http204-error/blob/master/test.nim#L12-L18
That's why in first case everything is works but in second case not.
For example you could adopt your second test case to

    let client = newHttpClient()
    let patchResponse = client.patch("http://127.0.0.1:8000/patch")
    check patchResponse.status == "204 No Content"

    let client2 = newHttpClient() # We creating new HttpClient object instance, so it will establish new connection to HTTP server.
    let response = client2.get("http://127.0.0.1:8000/ping") # We use `client2` instead of `client`.
    check response.status == "200 OK"
    check response.body == "pong"

[AuHau]

That is not the case! If I only change the response code from 204 to 200 and won't make the change you are suggesting, then the test is passing. See for the diff here:

https://github.com/AuHau/nim-http204-error/compare/200-version?expand=1

That is why I am wondering about your previous reasoning, because if it would be related to the HTTP pipelining, then it should also not pass right?

But it is possible that maybe the HttpClient has some conditions for PATH&204 cases? And thanks to the 200 code it handles this correctly?

[cheatfate]

That is not the case! If I only change the response code from 204 to 200 and won't make the change you are suggesting, then the test is passing. See for the diff here:

https://github.com/AuHau/nim-http204-error/compare/200-version?expand=1

That is why I am wondering about your previous reasoning, because if it would be related to the HTTP pipelining, then it should also not pass right?

But it is possible that maybe the HttpClient has some conditions for PATH&204 cases? And thanks to the 200 code it handles this correctly?

Please just check, its not an assumption its point why your test is incorrect. HTTP pipelining is DISABLED by default on REST server. Its why you can't use single HttpClient instance to perform 2 requests.

[AuHau]

I understand what you are saying, but then the tests should not pass with the 200 return code right? But they are passing. Can you please explain that? Is the HTTP pipelining disabled only for 204 return code?

If you run this PR, you can see that tests are passing: AuHau/nim-http204-error#1

[cheatfate]

Sorry but you dont understand what i'm saying. Your tests are incorrect you have your HttpClient object in faulty state after first connection attempt and its why you can't do another connection using HttpClient object which is already in faulty state.