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

Remove Connection: Close for REST api #2925

Open
wants to merge 2 commits into
base: general-devel
Choose a base branch
from

Conversation

sgkoishi
Copy link
Contributor

@sgkoishi sgkoishi commented Mar 8, 2023

fix #2923

drunderscore
drunderscore previously approved these changes Mar 9, 2023
Copy link
Contributor

@drunderscore drunderscore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable -- specifying the Connection header to begin with was a bit odd, don't think it's necessary or really beneficial anywhere.

Copy link
Member

@hakusaro hakusaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changelog?

@punchready
Copy link
Contributor

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
Copy link
Contributor Author

sgkoishi commented Mar 10, 2023

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

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

Successfully merging this pull request may close these issues.

Rest api cannot return complete data
4 participants