nanoframework / Home

:house: The landing page for .NET nanoFramework repositories.
https://www.nanoframework.net
MIT License
862 stars 79 forks source link

Http requests fails if the server response does not include a 'Content-Length' header #1394

Closed oleneveu closed 11 months ago

oleneveu commented 11 months ago

Library/API/IoT binding

System.Net.Http

Visual Studio version

N/A

.NET nanoFramework extension version

N/A

Target name(s)

ESP32_REV3

Firmware version

1.8.1.685

Device capabilities

No response

Description

When sending an http request to an http server using HttpClient or HttpWebRequest, if the server response includes a body and not a 'Content-Length' header, an exception is thrown when the response is parsed on the device.

Stack trace when using 'HttpClient':

    ++++ Exception System.Net.Sockets.SocketException - CLR_E_FAIL (1) ++++
    ++++ Message: 
    ++++ System.Net.Sockets.NativeSocket::recv [IP: 0000] ++++
    ++++ System.Net.Sockets.Socket::Receive [IP: 0018] ++++
    ++++ System.Net.Sockets.NetworkStream::Read [IP: 0062] ++++
    ++++ System.Net.InputNetworkStreamWrapper::ReadInternal [IP: 00d7] ++++
    ++++ System.Net.InputNetworkStreamWrapper::Read [IP: 000d] ++++
    ++++ System.Net.Http.StreamContent::SerializeToStream [IP: 0061] ++++
    ++++ System.Net.Http.HttpContent::LoadIntoBuffer [IP: 002a] ++++
    ++++ System.Net.Http.HttpClient::Send [IP: 006f] ++++
    ++++ System.Net.Http.HttpClient::Get [IP: 000c] ++++
    ++++ System.Net.Http.HttpClient::GetString [IP: 0006] ++++
    ++++ NanoframeworkHttpRequestIssue.Program::BeginSendHttpRequests [IP: 001a] ++++
    ++++ NanoframeworkHttpRequestIssue.Program::Main [IP: 0008] ++++
Exception thrown: 'System.Net.Sockets.SocketException' in System.Net.dll

How to reproduce

Send an http GET request to any endpoint that returns a body without including a 'Content-Length' header.

See repro project on GitHub (below)

Expected behaviour

No response

Screenshots

No response

Sample project or code

https://github.com/oleneveu/NanoframeworkHttpClientIssue

This repo contains two projects, each one with its own .sln :

Start the server and the client in debug mode. You should see the exception in the VS output. Now stop the server, uncomment line 40 to add the 'Content-Length' header to the response and restart the server. The client should now display the content of the response in the output.

In some cases, the client has to be restarted as the issue may cause some side effects (memory leaks?).

Aditional information

No response

oleneveu commented 11 months ago

I have read quickly the HttpClient implementation to see if I could find any clue regarding the cause of the issue. I am far from being confident that my analysis is correct, but for what I have understood there are two cases implemented to evaluate the length of the message body:

If I'm right, I think that a case is missing: An http server may send a response without any information about the length of the body, and close the connection when all the data has been sent.

See RFC 9112: https://www.rfc-editor.org/rfc/rfc9112#name-message-body-length :

  1. Otherwise, this is a response message without a declared message body length, so the message body length is determined by the number of octets received prior to the server closing the connection.

This behavior is quite common on embedded devices, as it often requires less resources to process the requests. It is also used to reduce the latency when streaming data.

In any case, my 'Philips Hue' bridge behaves like that...

Ellerbach commented 11 months ago

@oleneveu thanks, that does indeed help a lot. Do you have any example of a full response containing the elements? (you can obviously mask the real data but I'm interested in looking on how it looks like from the full response point of view with the headers, the Content-Lengh/Transfer-Encoding)

josesimoes commented 11 months ago

I've looked briefly the RFC you've pointed and there is this after 8. "Since there is no way to distinguish a successfully completed, close-delimited response message from a partially received message interrupted by network failure, a server SHOULD generate encoding or length-delimited messages whenever possible. The close-delimiting feature exists primarily for backwards compatibility with HTTP/1.0."

Which describes the problem very clearly. Without a content length and relying solely on the server closing the connection, there is no way of knowing if that was actually the server closing the connection or a network error producing the same outcome. This is not reliable at all as the application has no way of distinguishing a properly server terminated connection or a network error... 🤔

oleneveu commented 11 months ago

@Ellerbach I am not totaly sure of what you would like to have, so I have included two different responses: One with and one without a 'Content-Length' header. Responses with a 'Transfer-Encoding' header are more tricky to include here as they are usually using gzip and the body is binary.

If you don't already use it, I highly recommend that you try 'Telerik Fiddler': It is a tool that acts as an http proxy and that allows you to capture all the traffic that flows between your client and your server. You just have to configure your client to use it as a proxy. I've been using the free version ('Fiddler classic') for years, and I used it to capture the response that follows.

Here is a capture of a request to my hue bridge, that does not include a 'Content-Length' header :

HTTP/1.1 200 OK
Server: nginx
Date: Thu, 02 Nov 2023 15:06:26 GMT
Content-Type: application/json
Connection: close
Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0
Pragma: no-cache
Expires: Mon, 1 Aug 2011 09:00:00 GMT
Access-Control-Max-Age: 3600
Access-Control-Allow-Origin: *
Access-Control-Allow-Credentials: true
Access-Control-Allow-Methods: POST, GET, OPTIONS, PUT, DELETE, HEAD
Access-Control-Allow-Headers: Content-Type
X-XSS-Protection: 1; mode=block
X-Frame-Options: SAMEORIGIN
X-Content-Type-Options: nosniff
Content-Security-Policy: default-src 'self'
Cache-Control: no-store
Pragma: no-cache
Referrer-Policy: no-referrer

[{"error":{"type":3,"address":"/lights/3/state","description":"resource, /lights/3/state, not available"}}]

Here is a capture from another device that includes a 'Content-Length' header (I have removed the cookie):

HTTP/1.1 200 OK
Connection: keep-alive
Access-Control-Allow-Methods: GET, POST, OPTIONS
Access-Control-Allow-Origin: 
Access-Control-Allow-Credentials: true
Access-Control-Max-Age: 86400
Access-Control-Allow-Headers: Content-Type, Authorization
Set-Cookie: Auth-Type=http; SameSite=Lax
Set-Cookie: Auth-Token=<cookie_removed_from_here>; HttpOnly; SameSite=Lax
Content-Length: 585
Content-Type: application/json
ETag: "6589DB4E3A2F7E7074CBB738416D5C41"
Server: nzbget-21.1

{
"version" : "1.1",
"result" : "NZBGet.VersionInfo = {\n\t\"stable-version\": \"21.1\",\n\t\"stable-date\": \"2021-06-03\",\n\t\"stable-release-notes\": \"http:\/\/nzbget.net\/history-latest\",\n\t\"stable-update-rate\": \"0,0,0,1,2,3,5,10,20,30,40,70,100\",\n\t\"testing-version\": \"21.2-testing-r2333\",\n\t\"testing-date\": \"2021-10-01\", \n\t\"testing-release-notes\": \"http:\/\/nzbget.net\/history-latest-testing\",\n\t\"testing-update-rate\": \"0,0,0,1,2,3,5,10,20,30,40,70,100\",\n\t\"devel-release-notes\": \"https:\/\/github.com\/nzbget\/nzbget\/commits\/develop\"\n}\n"
}

Feel free to tell me if you need anything else.

oleneveu commented 11 months ago

@Ellerbach I've just checked the request sent by the nanoframework: it does not include an 'Accept-Encoding' header and enabling the compression does not seem to be an option. So you should never encounter a 'Transfer-Encoding' in the response.

oleneveu commented 11 months ago

@josesimoes This is true that on the 'transport' layer you cannot validate that the entire body was delivered. However, there are use cases where you can validate on the 'application' layer that you have received the whole payload. For instance, if you are transporting a JSON payload, you can ensure that it is valid by checking that it is correctly formatted. You can also have a custom validation mechanism in your application, for instance by using a specific marker at the end of the stream.

That reduces both the amount of data being transferred over the network and the latency when compared to using chunks.

Asp.Net core 7 web API can use that system to serialize an IEnumerable over an http connection without any buffering. If the source of data is slow, you can start reading the first item on the client before the second one has ever been written to the response on the server side.

You can even go further: Once the request header has been processed by the server, and the response header has been received by the client, you basically end up with a two-way TCP connection between the client and the server.

Ellerbach commented 11 months ago

Somehow, there is already a mechanism that is used to "guess" the size of the payload when it's not specified: https://github.com/nanoframework/System.Net.Http/blob/68421a7ef61eb118fe115719fe8515f1d7d48cb9/nanoFramework.System.Net.Http/Http/StreamContent.cs#L86

So this is maybe where there is a quick win or a bug. As when looking at the code, when not specified, it will try to read all the way. So something may be messed with the timeout or equivalent. @oleneveu as you have all what you need to test, if you clone the repo and use it and see why it's not behaving as ready all the way thru, you may be able to quickly fix it!

oleneveu commented 11 months ago

I saw that line when first analysing the issue, but I am not sure that anything can be solved from here. I think that the whole implementation was based on the assumption that the size of the data would be known.

When reading from the StreamContent, the stack is: StreamContent => InputNetworkStreamWrapper => NetworkStream => Socket => NativeSocket If the connection is closed before all the expected bytes are read, an exception is thrown by the native code through the NativeSocket methods.

If we want to change that behavior, we have to be able to detect that the connection was closed before trying to read on the socket.

So the steps to solve the issue might be :

  1. Identify how to detect a closed connection on the Socket class
  2. Add a 'Socket' property on the NetworkStream class
  3. Modify the InputNetworkStreamWrapper to check the status of the connection before reading the data
Ellerbach commented 11 months ago

There should be ways to check all this and add the case for unkonwn. As you mention there is a chain of elements and you can get to the very bottom of everything as there is a real socket behind :-)

oleneveu commented 11 months ago

The Socket instance was actually available directly from the InputNetworkStream, so the fix was easier than I expected.