Pryaxis / TShock

☕️⚡️TShock provides Terraria servers with server-side characters, anti-cheat, and community management tools.
GNU General Public License v3.0
2.41k stars 377 forks source link

Remove Connection: Close for REST api #2925

Open sgkoishi opened 1 year ago

sgkoishi commented 1 year ago

fix #2923

punchready commented 1 year ago

HTTP/1.1 defines the "close" connection option for the sender to signal that the connection will be closed after completion of the response. For example, Connection: close in either the request or the response header fields indicates that the connection SHOULD NOT be considered 'persistent' (section 8.1) after the current request/response is complete.

It doesn't seem like this should stop the response from being truncated, in fact this is rather the desired behavior as the REST server won't use the connection for any further communication line in a regular website visit.

sgkoishi commented 1 year ago

That header should not break things but it seems like the old library HttpServer, from .Net Framework 2.0 era, is not quite following - I reproduce the TCP RST before all transfers are completed correctly.

HttpServer.Messages.ResponseWriter.Send(IHttpContext, IResponse) tried to call Stream.Flush() to ensure all data were sent correctly, and then the stream is closed. The stream is an HttpServer.Transports.ReusableSocketNetworkStream, basically a wrapper over System.Net.Sockets.NetworkStream.

Add HttpServer.Logging.LogFactory.Assign(new HttpServer.Logging.ConsoleLogFactory(null)); to the plugin for more log:

2023/3/10 4:27:38AM.760 010 HttpContext.OnReceive                   CLIENTIP:CLIENTPORT received 408 bytes.
2023/3/10 4:27:38AM.762 010 HttpContext.OnReceive                   GET /test HTTP/1.1
Host: SERVER:7878
User-Agent: Many headers, ...

2023/3/10 4:27:38AM.764 010 HttpParser.Parse                        Parsing 408 bytes from offset 0 using ParseFirstLine
2023/3/10 4:27:38AM.767 010 MessageFactoryContext.OnRequestLine     GET: /test
2023/3/10 4:27:38AM.770 010 HttpParser.Parse                        Switched parser method to GetHeaderName at index 20
2023/3/10 4:27:38AM.770 010 HttpParser.Parse                        Switched parser method to GetHeaderValue at index 25
2023/3/10 4:27:38AM.771 010 MessageFactoryContext.OnHeader          Host: SERVER:7878
2023/3/10 4:27:38AM.774 010 HttpParser.Parse                        Switched parser method to GetHeaderName at index 47
2023/3/10 4:27:38AM.774 010 HttpParser.Parse                        Switched parser method to GetHeaderValue at index 58
2023/3/10 4:27:38AM.774 010 MessageFactoryContext.OnHeader          User-Agent: ...
2023/3/10 4:27:38AM.784 010 HttpParser.Parse                        Switched parser method to GetHeaderName at index 406
2023/3/10 4:27:38AM.784 010 HttpParser.Reset                        Resetting..
2023/3/10 4:27:38AM.787 010 HttpContext.OnRequest                   Received 'GET /test' from CLIENTIP:62451
2023/3/10 4:27:38AM.826 010 ResponseWriter.Send                     Sending 186 bytes.
2023/3/10 4:27:38AM.826 010 ResponseWriter.Send                     HTTP/1.1 200 Made by Jonas Gauffin
Content-Type: application/json; charset=utf-8
Content-Length: 28569
Connection: Close
Keep-Alive: timeout=5, max=100
Server: TShockAPI/5.1.3.0

2023/3/10 4:27:38AM.837 010 HttpParser.Reset                        Resetting..
2023/3/10 4:27:38AM.839 010 HttpParser.Parse                        Switched parser method to ParseFirstLine at index 408
2023/3/10 4:27:38AM.848 010 HttpContext.OnReceive                   Failed to read from stream: System.NullReferenceException: Object reference not set to an instance of an object.
   at HttpServer.HttpContext.OnReceive(IAsyncResult ar)
2023/3/10 4:27:38AM.849 010 ResponseWriter.Send                     Sending 135 bytes.
2023/3/10 4:27:38AM.849 010 ResponseWriter.Send                     HTTP/1.0 500 Object reference not set to an instance of an object.
Content-Type: text/html
Content-Length: 461
Connection: Close

2023/3/10 4:27:38AM.850 010 ResponseWriter.Send                     Failed to send data through context stream.
System.NullReferenceException: Object reference not set to an instance of an object.
   at HttpServer.Messages.ResponseWriter.Send(IHttpContext context, String data, Encoding encoding)
2023/3/10 4:27:38AM.850 010 ResponseWriter.SendBody                 Failed to send body through context stream.
System.NullReferenceException: Object reference not set to an instance of an object.
   at HttpServer.Messages.ResponseWriter.SendBody(IHttpContext context, Stream body)
2023/3/10 4:27:38AM.850 010 ResponseWriter.Send                     Failed to flush context stream.
System.NullReferenceException: Object reference not set to an instance of an object.
   at HttpServer.Messages.ResponseWriter.Send(IHttpContext context, IResponse response)

Note there are two responses for each REST (why two?).


image

With wireshark can confirm the RST is sent from the server side. Removing the closed fixed the problem so I didn't go deeper what's happening inside this library