RFR: JDK-8061729 : Update java/net tests to eliminate dependency on sun.net.www.MessageHeader and some other internal APIs

[Daniel Fuchs]

On Wed, 13 Oct 2021 22:04:23 GMT, Mahendra Chhipa <duke at openjdk.java.net> wrote:

> There are some regression tests depending on sun.net.www.MessageHeader, the internal API dependency should be removed. Some of other internal API dependancies are removed in following issues :
> JDK-8273142
> JDK-8268464
> JDK-8268133

It may be that the new library HttpHeaderParser works for the small use cases in which it is used, but its parsing of header fields is approximative. I am a bit uncomfortable by replacing a well tested parser (MessageHeader) with something that obviously doesn't implement the spec correctly - even though it's possible that the cases where it would give back a wrong answer do not occur in the scenario where it is used.

What is the underlying rationale for wanting to stop using MessageHeader in tests?

test/jdk/sun/net/www/protocol/http/NTLMTest.java line 163:

> 161: 
> 162:             for (int i=start; i<end; i++) {
> 163:                 MessageHeader header = new MessageHeader (s.getInputStream());

This change introduces a subtle behaviour difference in the test that may result in spurious "Connection Reset" exception. I would prefer if the existing logic was kept - and the test continued parsing the request headers before writing to the output.

test/lib/jdk/test/lib/net/HttpHeaderParser.java line 47:

> 45:             while(true) {
> 46:                 headerString = br.readLine();
> 47:                 if(headerString == null || headerString.isBlank()) {

This is not completely correct, a blank line would be an error, I think, or possibly an empty continuation line. isBlank() should be replaced by isEmpty().

test/lib/jdk/test/lib/net/HttpHeaderParser.java line 54:

> 52:                     List <String> values = new ArrayList<>();
> 53:                     String headerValue = headerString.substring(headerString.indexOf(": ") + 2);
> 54:                     String [] valueArray = headerValue.split(",");

This is not right either - at least that's not how MessageHeader (or the HttpClient or HttpServer header parsers work). Multi-valued header correspond to header fields whose name appear on several lines.


foo: bar, baz

   => single value "bar, baz"


foo: bar
foo: baz


multiple values => "bar", "baz"

Usually - analyzing the value - and possibly splitting it around `,` or `;` is done in the second step at a higher level.

test/lib/jdk/test/lib/net/HttpHeaderParser.java line 62:

> 60:                 } else {
> 61:                     requestDetails = headerString.trim();
> 62:                 }

This is not right: the request line - or status line - is the first line which is read - not just any line that doesn't contain `:`
FWIW, a request line may contain the full URL and therefore may contain `:`

test/lib/jdk/test/lib/net/HttpHeaderParser.java line 63:

> 61:                     requestDetails = headerString.trim();
> 62:                 }
> 63:             }

You should also take into account continuation lines (lines that begin with a space) - and IIRC a continuation would be \r followed by a space - so I'm not 100% sure that using readline() is completely appropriate as it will consume \r, \n, or \r\n equally.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5937