On Mon, 6 Jan 2025 05:43:31 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
> Can I please get a review of this change which proposes to address the > test-only issue noted in https://bugs.openjdk.org/browse/JDK-8347000? > > As noted in that issue, the test issues HTTP requests with `Content-Length` > set to `0` implying no request body. However, the code unintentionally sends > additional bytes (14 bytes each request) as part of the request. The JDK's > `HttpServer` implementation currently doesn't cause errors for such requests > (although the HTTP/1.1 RFC expects an error to be raised). > > The change in this PR merely addresses the test code to not send these > additional bytes. The test continues to pass after this change. While the change is reasonable, as it will avoid any ambiguous interpretation of the HTTP GET request used in the test, the rationale given for the change, is questionable, and based on a misinterpretation of the HTTP spec. That is to say: “As noted in that issue, the test issues HTTP requests with Content-Length set to 0 implying no request body. However, the code unintentionally sends additional bytes (14 bytes each request) as part of the request. The JDK's HttpServer implementation currently doesn't cause errors for such requests (although the HTTP/1.1 RFC expects an error to be raised).” is not totally correct -- "although the HTTP/1.1 RFC expects an error to be raised", raising of an "error" relates to User Agent behaviour, not the server. The HttpServer used in the test is parsing the GET request correctly and interpreting the Content-Length: 0 correctly, as there is no request body associated with the request, when presented to the HttpHandler. This is can be observed by instrumenting the HttpHandler’s handle method and checking for any request body data. The delimiting of the test’s GET request is Content-Length:0 and the two blank lines. These are the key inputs when parsing a request, hence the additional data written on the HTTP content is not relevant. The stray bytes might cause a problem if a JDK HttpServer were processing a “stream of requests”. But, it would be expected that the server input processing to discard any stray data until to reads a valid request method. Additionally, based on your assumptions, above, an HTTP user agent would not be able to submit consecutive HTTP GET request (with Content-Length: 0 ) on the same TCP connection, to a JDK HttpServer. The change is fine, but I would suggest correcting your assumption in the context of the test. ------------- PR Comment: https://git.openjdk.org/jdk/pull/22921#issuecomment-2573632989
