On Fri, 11 Feb 2022 11:15:56 GMT, Mahendra Chhipa 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
>
> Mahendra Chhipa has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 1. Handled continuation line
> 2. handled \r, \n, or \r\n
> 3. Added the test for HttpHeaderParser class
test/lib/jdk/test/lib/net/HttpHeaderParser.java line 100:
> 98: public List getHeaderValue(String key) {
> 99: if(headerMap.containsKey(key.toLowerCase())) {
> 100: return headerMap.get(key.toLowerCase());
I'd suggest to use `key.toLowerCase(Locale.ROOT)` - even though it shouldn't
really matter if it's guaranteed that keys have been syntactically checked for
illegal characters.
test/lib/jdk/test/lib/net/HttpHeaderParser.java line 120:
> 118: * ( when true is returned ), which may not be fully consumed.
> 119: *
> 120: * @param input the ( partial ) header data
It doesn't look like this is a accurate. There's no ByteBuffers anywhere...
test/lib/jdk/test/lib/net/HttpHeaderParser.java line 148:
> 146: case FINISHED -> false;
> 147: case STATUS_LINE_FOUND_LF, STATUS_LINE_END_LF,
> HEADER_FOUND_LF -> true;
> 148: default -> buffer.available() >= 0;
It's a bit chancy to use InputStream.available() - it would be better to have
some `eof` variable that's set to true when EOF is reached, and return `!eof`
here
test/lib/jdk/test/lib/net/HttpHeaderParser.java line 162:
> 160: * value from [0, 255] representing the character code.
> 161: *
> 162: * @param input a {@code ByteBuffer} containing a partial input
Input is not a ByteBuffer and it doesn't contain a partial input.
test/lib/jdk/test/lib/net/HttpHeaderParser.java line 171:
> 169: private char get(InputStream input) throws IOException {
> 170: return (char)(input.read() & 0xFF);
> 171: }
That doesn't look right as it will prevent you to read EOF. Maybe it should be:
boolean eof;
...
private int get(InputStream input) throws IOException {
int c = input.read();
if (c < 0) eof = true;
return c;
}
And then you would have to check for EOF everywhere where `get(input)` is
called too.
test/lib/jdk/test/lib/net/HttpHeaderParser.java line 175:
> 173: private void readResumeStatusLine(InputStream input) throws
> IOException {
> 174: char c = 0;
> 175: while (input.available() > 0 && (c = get(input)) != CR) {
It's not a too good idea to depend on input.available() - the spec leaves too
much room for odd behaviors.
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/InputStream.html#available()
An implementation may decide to return 0 always - as it is the amount of bytes
that is guaranteed to be readable without blocking (and an implementation may
not know how many bytes can be read without blocking).
-
PR: https://git.openjdk.java.net/jdk/pull/5937
