Response Status Line parsing is error prone

[Oliver-Loeffler]

With current implementation in HttpResponseReader in case of invalid messages there would be something like an IllegalArgumentError etc. Here it would be better to possibly return an Optional with a HttpResponse embedded when the response is valid. If response line is invalid, returning an empty Optional would be okay.

Consumers the can be called by:

HttpResponse.fromBytes(byte[] bytes).ifPresent(Consumer<HttpResponse> consumer)

Response status line is defined in http://httpwg.org/specs/rfc7230.html#status.line.

A valid response status line:

• has no preceding empty lines
• has no leading spaces
• follows: status-line = HTTP-version SP status-code SP reason-phrase CRLF
• with status being a 3 digit integer
• with reason-phrase being a string followed by a line terminator (carriage return, line feed)

Placing this into separate class / method would make it testable. Here similar approach would be possible for Request Line.

[hendrikebbers]

Mhhh, I don't know if it really would make sense to do it that way. If you receive some invalid messages on a connection that should be a HTTP connection I would define this as a broken connection.

In addition using a byte[] as an inout parameter is wrong in my point of view. The input should always be a NIO channel. Only by doing it that way a real streaming of HTTP messages can be created.

Example:

1: Create Channel (Connect to endpoint)
2: Create executor / thread that handles this channel
3: In executor: Read from channel until it is closed
4: In executor: Each complete HTTP message that was read will be passed to the consumer.

Based on this the API will look more like:

HttpResponse.readFromChannel(Channel c, Consumer<HttpResponse> consumer)
I think it do not make sense to allow an API without defining a consumer since in that case the date of the channel will not be needed and it do not make sense to call the method at all.

Another option would be the use of a stream. In that case the API would look like this:

Stream< HttpResponse> stream = HttpResponse.readFromChannel(Channel c);
Maybe it would make sense to provide both methods. By doing so a developer can decide if he wants to work with a stream or with a consumer. By using the stream we have the additional feature that the end of the connection can be handled in a easy way (stream is done). If we use the Consumer we need to define such an API. This can be done by the return value of the return value of the method:

Future<Void> channelFuture = HttpResponse.readFromChannel(Channel c, Consumer<HttpResponse> consumer);

try {
   channelFuture.get();
   // Channel was closed without an error (all messages could be parsed)
} catch(Exception e) {
   //Channel was closed by exception or a parse error occurred while handling the stream
}

By using the the stream API we do not have a change to handle an error without adding a error handler as a param to the method:

Stream< HttpResponse> stream = HttpResponse.readFromChannel(Channel c, ErrorHandler e);

So several possibilities :) What do you think?

[Oliver-Loeffler]

I fully agree with the concept, its actually a good example.
Reading generally from channels and handing over consumers is a very good idea, especially by means of API consistency. Internally, reading from bytes[] is still needed, or?

Providing a stream of HttpResponses is also a good idea, you probably have some use case in mind here. Having the option to work with java Future<Void> channelFuture is also quite a good idea as this should be a good leveraging the non-blocking capabilities.
I think its worth to start with the stream output, I need to play with futures, I know the idea behind but never used them.

Nevertheless the parsing as it happens right now (no matter which branch) is not really nice, the more I read from HTTP the more different I would like to do it. Which means that a HttpResponse could be separated into several internal, functional classes:

HttpResponse could incorporate:

• a ResponseLine type (which basically will keep protocol version, status code, response phrase) and it will provide something like:

 boolean validity = new ResponseLine(...).isValid()

Unexpected errors could be wrapped into a ResponseParsingException (unchecked) or so.

• a Set whereas each HeaderField will have a name, a value and also an .isValid() method; there might be a subset of standard fields which will be addressable by using the existing enums (ResponseFields, EntityFields, GeneralFields) but in opposite to now there won't be a map holding the fields anymore. HeaderField should incorporate all basic rules and semantics from RFC.

• same for message body, as above mentioned for header fields and request line.

I was playing with different requests and tested multiple response types and the current HttpResponse.read*(...) failed with ugly errors multiple times. Those internal classes would also have the advantage to provide meaning full error types. Or is that too overblown?

As there is no "public" API yet, we're still free to shape it.

[Oliver-Loeffler]

Instead of a .isValid() method, the new StatusLine type throws a HttpMessageParsingExceptionwhen the message being parsed is invalid.

[Oliver-Loeffler]

HttpResponseReader.fromChannel(...)returns now a FutureTask<Void>, there is also now a small demo using this FutureTask together with an ExecutorService.