Provide a better way to handle decoder failures

[naude-r]

hi,

Have run into a problem when issuing a malformed HTTP request, e.g. GET /blah.

Netty generates an exception:
java.lang.IllegalArgumentException: empty text

This is correct, but one is not allowed to return an error message. Generating a BAD_REQUEST response results in another exception:
java.lang.IllegalStateException: cannot send more responses than requests

I guess this is due to a request never been generated? Additionally one cannot log an access record as the http request information is not available when receiving the exception in exceptionCaught.

What would be the best way to resolve this?

[jpinner]

I usually resolve this by catching the exception and passing a
"BAD_REQUEST" upstream. See:

https://github.com/twitter/finagle/blob/master/finagle-http/src/main/scala/com/twitter/finagle/http/Codec.scala

Hope this helps.

On Fri, Jul 6, 2012 at 2:53 AM, naude-r
reply@reply.github.com
wrote:

hi,

Have run into a problem when issuing a malformed HTTP request, e.g. GET /blah.

Netty generates an exception:
java.lang.IllegalArgumentException: empty text

This is correct, but one is not allowed to return an error message. Generating a BAD_REQUEST response results in another exception:
java.lang.IllegalStateException: cannot send more responses than requests

I guess this is due to a request never been generated? Additionally one cannot log an access record as the http request information is not available when receiving the exception in exceptionCaught.

What would be the best way to resolve this?

---

Reply to this email directly or view it on GitHub:
#441

[normanmaurer]

@jpinner, @trustin I wonder if we should somehow make this "easier"

[normanmaurer]

@naude-r btw could you please post the stacktraces here.. thanks!

[naude-r]

The issue can triggered with the HttpStaticFileServer example:
telnet localhost 8080
GET /somefile
Host: localhost

java.lang.IllegalArgumentException: empty text
        at org.jboss.netty.handler.codec.http.HttpVersion.<init>(HttpVersion.java:97) ~[netty-3.5.2.Final.jar:na]
        at org.jboss.netty.handler.codec.http.HttpVersion.valueOf(HttpVersion.java:62) ~[netty-3.5.2.Final.jar:na]
        at org.jboss.netty.handler.codec.http.HttpRequestDecoder.createMessage(HttpRequestDecoder.java:76) ~[netty-3.5.2.Final.jar:na]
        at org.jboss.netty.handler.codec.http.HttpMessageDecoder.decode(HttpMessageDecoder.java:189) ~[netty-3.5.2.Final.jar:na]
        at org.jboss.netty.handler.codec.http.HttpMessageDecoder.decode(HttpMessageDecoder.java:101) ~[netty-3.5.2.Final.jar:na]
        at org.jboss.netty.handler.codec.replay.ReplayingDecoder.callDecode(ReplayingDecoder.java:502) ~[netty-3.5.2.Final.jar:na]
        at org.jboss.netty.handler.codec.replay.ReplayingDecoder.messageReceived(ReplayingDecoder.java:437) ~[netty-3.5.2.Final.jar:na]
        at org.jboss.netty.channel.Channels.fireMessageReceived(Channels.java:268) ~[netty-3.5.2.Final.jar:na]
        at org.jboss.netty.channel.Channels.fireMessageReceived(Channels.java:255) ~[netty-3.5.2.Final.jar:na]
        at org.jboss.netty.channel.socket.nio.NioWorker.read(NioWorker.java:91) ~[netty-3.5.2.Final.jar:na]

[naude-r]

@jpinner thank you. the work-around works just fine.

[jpinner]

@trustin, @normanmaurer would it be preferable to generate a "bad request" object in the request decoder and pass the object upstream (letting the user determine the error response, perform logging, etc.) or to catch the exception in the server codec and return an appropriate error response ourselves (400, 413, 414, etc.)

[trustin]

@jpinner I prefer to geretate a 'bad request' object in the request decoder.

[normanmaurer]

@trustin @jpinner me too.. But I'm a bit worried about such a change in 3.5.x as it may break existing applications. What about to target it for 3.6.x ?

[trustin]

What about 4?

[normanmaurer]

@trustin also ok :)

[trustin]

Working on this. The idea is, when decoding fails, a bad http message is generated so that an inbound handler can handle it. Any subsequent inbound traffic should be discarded by the decoder. Sometimes, the handler might be expecting HttpChunk rather than HttpRequest/HttpResponse, so I think we should introduce a new supertype for HttpMessage and HttpChunk. The resulting type hierarchy could be:

interface HttpObject
interface HttpMessage extends HttpObject
interface HttpChunk extends HttpObject
interface BadHttpObject extends HttpObject
interface BadHttpMessage extends BadHttpObject
interface BadHttpChunk extends BadHttpObject

What do you think? BadHttpObject could provide the cause of the decode failure. A user might want to determine if the decode failed while decoding a chunk or not.

[jpinner]

In SPDY we indicate failure with a field in the object itself (see isInvalid method in SpdyHeaderBlock). I think we should do something similar here because of the following:

When we encounter an error we may want to pass / collect / generate / parse / etc. as much data as possible so that upstream handlers can log the cause of the error (maybe for statistics on bad clients or debug errors in the decoder). If we have already generated the request object and started decoding, we don't want to have to necessarily generate a new object and copy the data into it just to satisfy the class hierarchy. Consider the case of adding headers to a request object until exceeding the maximum header size and then deciding to declare the request as "bad." We may want to log the headers already received or the URI of the request and otherwise we would have to generate a new "bad" request object and copy the data or save the original request as a field within it. There is less wasted work by being able to simply "mark" the object as bad and perhaps given a reason (a response code for instance that can be used to indicate the type of error).

[trustin]

Check out the updated examples: 045b621

[slandelle]

Hi,

I have the feeling that this issue also happens in 3.6: AsyncHttpClient/async-http-client#93
If so, any chance of having the fix backported?

Cheers,

Stéphane

[ThE-MaRaC]

Hi,

with the correction, output of the toString() method has been changed, content is not returned any more, what was the reason for this change?

Thx.

Regards,
ThE-MaRaC

[trustin]

@ThE-MaRaC Whose toString()?