Ending a HttpServerResponse that raised an exception?

[Francois Delisle]

If an exception is raised on a HttpServerResponse, such that its exceptionHandler is invoked, is it still required/appropriate to end the response by invoking response.end(). Would the bodyEndHandler() still be called?

I am using the bodyEndHandler() of a response to cleanup resources I created while handling the request. However, I always need to cleanup these resources, even in case of an error. I'm wondering if I am supposed to clean up these resources in a provided exceptionHandler directly, in addition to the bodyEndHandler I have, or if I can simply end the response in the exceptionHandler such that my bodyEndHandler is invoked.

1) Handle in both:

response.exceptionHandler( ... cleanup resources );

response.bodyEndHandler( ... cleanup resources );

2) End response on exception:

response.exceptionHandler( response.end(); ); // normally these exceptions are because something really wrong happens with the connection, so there is no need to specify a status code or status message since it can't be written

   [response.end() invokes the bodyEndHandler());

response.bodyEndHandler( ... cleanup resources );

If we are supposed to always end a response, even when an error occurred on the underlying connection and there is no chance we can write the response to the wire, then solution 2 is quite elegant.

As a reference, I looked at the TimeoutHandler provided by vertx-web. It uses the bodyEndHandler() to cancel the timer. If there is an error on the connection (dispatched to the response), by default the timer would not be canceled. However, if the expectation is that response.end() should be called even on "fatal" errors (connection is unusable), then it makes sense.

Interestingly, I also want to release resources when the connection is closed unexpectedly. I register a closeHandler() on the response. I was able to test that it is safe to call response.end() when that handler is invoked, even if the underlying connection is obviously closed. However, I could not find a way to cause an exception to occur on a connection.

Feedback would be greatly appreciated.

[Julien Viet]

On Feb 24, 2017, at 7:27 PM, Francois Delisle <francois...@gmail.com> wrote:

If an exception is raised on a HttpServerResponse, such that its exceptionHandler is invoked, is it still required/appropriate to end the response by invoking response.end(). Would the bodyEndHandler() still be called?

no, bodyEndHandler is called by Vert.x when the last chunk of the response has been sent, so at this moment you should not attempt to use the HttpServerResponse anymore.

I am using the bodyEndHandler() of a response to cleanup resources I created while handling the request. However, I always need to cleanup these resources, even in case of an error. I'm wondering if I am supposed to clean up these resources in a provided exceptionHandler directly, in addition to the bodyEndHandler I have, or if I can simply end the response in the exceptionHandler such that my bodyEndHandler is invoked.

1) Handle in both:

response.exceptionHandler( ... cleanup resources );

response.bodyEndHandler( ... cleanup resources );

it depends on the protocol:

1/ with HTTP/1.x you should set also the response.closeHandler() that is called if the connection is closed before you send the entire response

2/ with HTTP/2 when the connection is closed before the entire response is sent, the exceptionHandler

I believe it should be more consistent and somehow the exceptionHandler should be called for HTTP/1.x, at least if no close handler is set. That would be more consistent with HttpClient that behaves like this (HttpClientResponse does not have a closeHandler).

Actually I think that closeHandler should be deprecated on HttpServerResponse in favor of HttpConnection#closeHandler and if something wrong happens when an HttpServerResponse is not finished, the exception handler should be called (like it does for HTTP/2).

That would ease the implementation of server that handle both HTTP/1 and HTTP/2 .

2) End response on exception:

response.exceptionHandler( response.end(); ); // normally these exceptions are because something really wrong happens with the connection, so there is no need to specify a status code or status message since it can't be written

   [response.end() invokes the bodyEndHandler());

response.bodyEndHandler( ... cleanup resources );

If we are supposed to always end a response, even when an error occurred on the underlying connection and there is no chance we can write the response to the wire, then solution 2 is quite elegant.

as said earlier if something wrong happens you should not touch anymore the response.

As a reference, I looked at the TimeoutHandler provided by vertx-web. It uses the bodyEndHandler() to cancel the timer. If there is an error on the connection (dispatched to the response), by default the timer would not be canceled.

I think it can happen that we don’t cancel the timeout but if we don’t the consequence has limited effects.

However, if the expectation is that response.end() should be called even on "fatal" errors (connection is unusable), then it makes sense.

Interestingly, I also want to release resources when the connection is closed unexpectedly. I register a closeHandler() on the response. I was able to test that it is safe to call response.end() when that handler is invoked, even if the underlying connection is obviously closed. However, I could not find a way to cause an exception to occur on a connection.

you can set the HttpServerResponse to be chunked, then send a single chunk. When the other side gets the chunk it closes the connection.

Feedback would be greatly appreciated.

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at https://groups.google.com/group/vertx.
To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/9c63f8cc-3435-4cd2-9e1b-5032d8af5f32%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Francois Delisle]

On Friday, February 24, 2017 at 4:37:43 PM UTC-5, Julien Viet wrote:

On Feb 24, 2017, at 7:27 PM, Francois Delisle <francois...@gmail.com> wrote:

If an exception is raised on a HttpServerResponse, such that its exceptionHandler is invoked, is it still required/appropriate to end the response by invoking response.end(). Would the bodyEndHandler() still be called?

no, bodyEndHandler is called by Vert.x when the last chunk of the response has been sent, so at this moment you should not attempt to use the HttpServerResponse anymore.

I understand I should not try to use the response after the bodyEndHandler has been invoked. In fact trying to do so would result in a exception (checkWritten()). What I was asking is if it would make sense to call response.end() when the response invokes its registered exceptionHandler.

Looking at the code (HttpServerResponse.end() from vertx 3.3.3), it seems the last chunk is scheduled to be written on the channel but the bodyEndHandler is called immediately, in the same event cycle, no matter if writing this last chunk will succeed or not. I'm looking more specifically at HttpServerResponse.end0(); If the underlying connection is already closed (as a result of a premature close which invoked the closeHandler), it seems the request to write will simply be ignored (no error raised). If an exception was already reported through the exceptionHandler, it is unclear to me what happens. But again, looking at the code, if there is an exception writing to the channel, it seems the exception handler of the response is notified but the connection stays open. In other words, vertx doesn't automatically close the underlying connection of a response in the event an exception is raised. Am I wrong? 

If we imagine the following scenario, in the context of HTTP 1.x

1) Server calls response.write( buffer ); which queues the buffer for write

2) Some time later, the buffer is written to the channel. An exception is raised since the buffer cannot be written for some reason. Assume that the connection is still opened because the exception was not due to a closed connection (perhaps this is a bad assumption?)

3) The exception handler of the response is invoked. If I understood correctly, you said the response is essentially dead and unusable and I should not use it anymore. 

This confuses me because I don't understand

1) what will close the underlying connection for a non-keep-alive connection

2) what will clear the "active request" on the connection for a keep-alive connection so that the connection can be reused for other requests.

3) how will the other end (the client waiting for the response) be notified of the error? 

Are we saying that when there is an exception raised by a server response (or request), the underlying connection is considered dead and there is no cleanup to do. It will be closed automatically and you should simply abort the request handling and cleanup resources. The bodyEndHandler won't be called and this is by design since the request essentially failed?

[Francois Delisle]

Let think one abstraction level above, and see the HttpServerResponse as a WriteStream. When I compare it with AsyncFile which is also a WriteStream, I can clearly see that if the AsyncFile invokes its exceptionHandler, the AsyncFile still needs to be closed manually otherwise its underlying file channel will leak. Also, it seems the AsyncFile could invoke its exceptionHandler multiple times if you simply retry reading again. Is there an hidden contract in general that says that if a Stream (read or write) raises an exception, this stream is dead, unusable and you should close it immediately?

If an exception is raised by a HttpServerResponse, should I close it immediately, which will close its underlying connection (even for HTTP 2.x?).

[Julien Viet]

On Feb 24, 2017, at 11:18 PM, Francois Delisle <francois...@gmail.com> wrote:

On Friday, February 24, 2017 at 4:37:43 PM UTC-5, Julien Viet wrote:

On Feb 24, 2017, at 7:27 PM, Francois Delisle <francois...@gmail.com> wrote:

If an exception is raised on a HttpServerResponse, such that its exceptionHandler is invoked, is it still required/appropriate to end the response by invoking response.end(). Would the bodyEndHandler() still be called?

no, bodyEndHandler is called by Vert.x when the last chunk of the response has been sent, so at this moment you should not attempt to use the HttpServerResponse anymore.

I understand I should not try to use the response after the bodyEndHandler has been invoked. In fact trying to do so would result in a exception (checkWritten()). What I was asking is if it would make sense to call response.end() when the response invokes its registered exceptionHandler.

Looking at the code (HttpServerResponse.end() from vertx 3.3.3), it seems the last chunk is scheduled to be written on the channel but the bodyEndHandler is called immediately, in the same event cycle, no matter if writing this last chunk will succeed or not. I'm looking more specifically at HttpServerResponse.end0(); If the underlying connection is already closed (as a result of a premature close which invoked the closeHandler), it seems the request to write will simply be ignored (no error raised). If an exception was already reported through the exceptionHandler, it is unclear to me what happens. But again, looking at the code, if there is an exception writing to the channel, it seems the exception handler of the response is notified but the connection stays open. In other words, vertx doesn't automatically close the underlying connection of a response in the event an exception is raised. Am I wrong? 

when the connection is closed and there is an in-flight response, the response close handler is called.

If we imagine the following scenario, in the context of HTTP 1.x

1) Server calls response.write( buffer ); which queues the buffer for write

2) Some time later, the buffer is written to the channel. An exception is raised since the buffer cannot be written for some reason. Assume that the connection is still opened because the exception was not due to a closed connection (perhaps this is a bad assumption?)

3) The exception handler of the response is invoked. If I understood correctly, you said the response is essentially dead and unusable and I should not use it anymore. 

This confuses me because I don't understand

1) what will close the underlying connection for a non-keep-alive connection

Vert.x will close the connection after the content is written.

2) what will clear the "active request" on the connection for a keep-alive connection so that the connection can be reused for other requests.

when the last bytes of the http request have been received, the current request will be cleared, the corresponding response is usually not yet finished.

3) how will the other end (the client waiting for the response) be notified of the error? 

it will get its connection closed

Are we saying that when there is an exception raised by a server response (or request), the underlying connection is considered dead and there is no cleanup to do. It will be closed automatically and you should simply abort the request handling and cleanup resources. The bodyEndHandler won't be called and this is by design since the request essentially failed?

usually these errors are not recoverable.

the cleanup should be done in closeHandler / exceptionHandler of HttpServerResponse

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at https://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/c255f662-9c03-4154-af0a-2d5cd02b64eb%40googlegroups.com.

[Julien Viet]

On Feb 24, 2017, at 11:49 PM, Francois Delisle <francois...@gmail.com> wrote:

Let think one abstraction level above, and see the HttpServerResponse as a WriteStream. When I compare it with AsyncFile which is also a WriteStream, I can clearly see that if the AsyncFile invokes its exceptionHandler, the AsyncFile still needs to be closed manually otherwise its underlying file channel will leak. Also, it seems the AsyncFile could invoke its exceptionHandler multiple times if you simply retry reading again. Is there an hidden contract in general that says that if a Stream (read or write) raises an exception, this stream is dead, unusable and you should close it immediately?

no there is not, some stream may be recoverable, some not depending on the stream.

If an exception is raised by a HttpServerResponse, should I close it immediately, which will close its underlying connection (even for HTTP 2.x?).

no the server should take care of this for you.

On Friday, February 24, 2017 at 5:18:03 PM UTC-5, Francois Delisle wrote:

On Friday, February 24, 2017 at 4:37:43 PM UTC-5, Julien Viet wrote:

On Feb 24, 2017, at 7:27 PM, Francois Delisle <francois...@gmail.com> wrote:

If an exception is raised on a HttpServerResponse, such that its exceptionHandler is invoked, is it still required/appropriate to end the response by invoking response.end(). Would the bodyEndHandler() still be called?

no, bodyEndHandler is called by Vert.x when the last chunk of the response has been sent, so at this moment you should not attempt to use the HttpServerResponse anymore.

I understand I should not try to use the response after the bodyEndHandler has been invoked. In fact trying to do so would result in a exception (checkWritten()). What I was asking is if it would make sense to call response.end() when the response invokes its registered exceptionHandler.

Looking at the code (HttpServerResponse.end() from vertx 3.3.3), it seems the last chunk is scheduled to be written on the channel but the bodyEndHandler is called immediately, in the same event cycle, no matter if writing this last chunk will succeed or not. I'm looking more specifically at HttpServerResponse.end0(); If the underlying connection is already closed (as a result of a premature close which invoked the closeHandler), it seems the request to write will simply be ignored (no error raised). If an exception was already reported through the exceptionHandler, it is unclear to me what happens. But again, looking at the code, if there is an exception writing to the channel, it seems the exception handler of the response is notified but the connection stays open. In other words, vertx doesn't automatically close the underlying connection of a response in the event an exception is raised. Am I wrong? 

If we imagine the following scenario, in the context of HTTP 1.x

1) Server calls response.write( buffer ); which queues the buffer for write

2) Some time later, the buffer is written to the channel. An exception is raised since the buffer cannot be written for some reason. Assume that the connection is still opened because the exception was not due to a closed connection (perhaps this is a bad assumption?)

3) The exception handler of the response is invoked. If I understood correctly, you said the response is essentially dead and unusable and I should not use it anymore. 

This confuses me because I don't understand

1) what will close the underlying connection for a non-keep-alive connection

2) what will clear the "active request" on the connection for a keep-alive connection so that the connection can be reused for other requests.

3) how will the other end (the client waiting for the response) be notified of the error? 

Are we saying that when there is an exception raised by a server response (or request), the underlying connection is considered dead and there is no cleanup to do. It will be closed automatically and you should simply abort the request handling and cleanup resources. The bodyEndHandler won't be called and this is by design since the request essentially failed?

-- 
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at https://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/00fb96d2-6da9-461a-bc33-e0ea9cbc07d9%40googlegroups.com.

[Julien Viet]

Overall, I believe there should be an HttpServerResponse endHandler/doneHandler that is called consistently after the response has ended whatsoever.

Because HttpServerResponse#closeHandler does not have the same meaning for HTTP/1 (it is called if the underlying connection is closed before the request ended) and HTTP/2 (it is called when the related stream is called, always).

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at https://groups.google.com/group/vertx.

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/c255f662-9c03-4154-af0a-2d5cd02b64eb%40googlegroups.com.

[Julien Viet]

or maybe the closeHandler can be changed to be called anyway in HTTP/1, that would save to add one handler.

if some code needs to be aware of connection close it can call connection().closeHandler()

[Francois Delisle]

I agree with the vision, assuming I followed correctly:

1) HttpServerResponse's closeHandler (or a new "done" handler to avoid breaking existing apps and go through a deprecation process instead) would always be invoked when the response lifetime completes, whether it succeeded or not.

The handler would also be called if the underlying connection closed prematurely, as it is the case already, since this also completes the lifetime of the request.

This would align the contract of HTTP 1.x and 2 server responses for the close handler.

This would make it the ideal handler for the application to cleanup resources whose lifetime matches the lifetime of the request/response exchange.

It would be the equivalent of a "finally" block in the familiar try-catch-finally construct.

2) HttpServerResponse's exceptionHandler would be called on an error, but also when its underlying connection closed prematurely (before the response was fully written). 

This would align the contract of HttpServerResponse for HTTP 1.x and 2 for the exception handler, and make it consistent with HttpClientResponse.

An application would typically log the exception and react accordingly but would not have to release resources in this case (assuming it installed the close/done handler mentioned in point 1).

It would be the equivalent of a "catch" block in a try-catch-finally construct. 

Until these changes are made (if they ever make it!), however, an application needs to release its resources...

a) when the response succeeded, in the bodyEndHandler

b) when the response failed because of an exception, in the exceptionHandler

c) when the response failed because its underlying connection closed prematurely, in the closeHandler

In any case (present or future), an application does not have to release the HttpServerResponse and/or its underlying connection, on success or failure. Vert.x takes care of it.

a) When the connection was closed, obviously there is no need to close the connection.

b) When an exception was raised on the response, somehow the response and its underlying connection are "released". I could not find a definitive proof of this in the code and failed at trying to cause such an exception (I'm talking an exception raised on the ServerConnection by the netty's underlying channel, dispatched to the response, which is not necessarily caused by the connection being closed).

Finally, in case of a failure (exception, connection closed prematurely), the application should not use the response anymore. So my original idea of ending the response when its exception handler is invoked is wrong.

Hopefully I got that right.

I greatly appreciate you taking the time to help me understand these contracts ensuring adequate error handling in my first service application based on Vert.x

[Julien Viet]

On Feb 26, 2017, at 6:21 AM, Francois Delisle <francois...@gmail.com> wrote:

I agree with the vision, assuming I followed correctly:

1) HttpServerResponse's closeHandler (or a new "done" handler to avoid breaking existing apps and go through a deprecation process instead) would always be invoked when the response lifetime completes, whether it succeeded or not.

The handler would also be called if the underlying connection closed prematurely, as it is the case already, since this also completes the lifetime of the request.

This would align the contract of HTTP 1.x and 2 server responses for the close handler.

This would make it the ideal handler for the application to cleanup resources whose lifetime matches the lifetime of the request/response exchange.

It would be the equivalent of a "finally" block in the familiar try-catch-finally construct.

correct, I’ve created a branch with that https://github.com/eclipse/vert.x/tree/close-handler-modifications

2) HttpServerResponse's exceptionHandler would be called on an error, but also when its underlying connection closed prematurely (before the response was fully written). 

This would align the contract of HttpServerResponse for HTTP 1.x and 2 for the exception handler, and make it consistent with HttpClientResponse.

An application would typically log the exception and react accordingly but would not have to release resources in this case (assuming it installed the close/done handler mentioned in point 1).

It would be the equivalent of a "catch" block in a try-catch-finally construct. 

on a close (channel inactive), I would rather only call the close handler as it is the case today.

i would rather modify HttpClientResponse (or client request maybe, see below) to add a similar closeHandler

because on HttpClient you need to set both an handler on the request and on the response to handle all the case (think the client sends a big payload), which makes difficult to release resources.

Until these changes are made (if they ever make it!), however, an application needs to release its resources...

a) when the response succeeded, in the bodyEndHandler

b) when the response failed because of an exception, in the exceptionHandler

c) when the response failed because its underlying connection closed prematurely, in the closeHandler

correct

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/e55794e7-b822-4871-867f-e816648971c4%40googlegroups.com.

[Julien Viet]

so initially I went with a modification of the closeHandler to be called but then I realized that some code relies on the fact that closeHandler is only called when connection is closed before the end of the response.

so I’ve added an “endHandler” that is called when the response ends whatsoever and aims to perform cleanup like operations.

closeHandler is called whenever the transport (connection for HTTP/1.x and Stream for HTTP/2) is called and the connection is still using the connection somehow which is the actual behavior.

Julien

[Francois Delisle]

That's great. I mentioned earlier that changing the contract of the closeHandler() would break backward compatibility.

I'm "emulating" this callback until I can get my hands on it (I call it the completionHandler).

   response's closeHandler invokes completionHandler.handle()

   response's bodyEndHandler invokes completionHandler.handle()

   response's exceptionHandler invokes response.close() // this will call eventually call its closeHandler if the connection was not closed already, then calling the completionHandler

   request's exceptionHandler invokes response.close() // .. same as above.. but in the case of the request, there are really case where the requestion can raise an exception by itself (e.g. while decoding the request data) while the underlying connection/channel is still on. 

This completionHandler cannot be called twice since response.closeHandler() is only called if the connection was closed while there was still a pending request.

This pending request is nullified when it is ended, in which case the bodyEndHandler is invoked.

One problem I'm facing right now is that part of the cleanup I'm doing consists in calling pump.stop() on a pump whose writeStream is the HttpServerResponse.

This way, if the response completes because of a failure (exception or premature close on connection), the pumping is stopped by nullifying the readstream's handler.

However, if everything went well and a response got written, pump.stop() fails because it tries to nullify the drainHandler on the HttpServerResponse.

In this case, the response checkWritten() throws an exception. I feel the HttpServerResponse should tolerate having null handlers being set even after it has ended...

So to keep things simple, I'm thinking of simply create a safeStopPump() method that will workaround this specific problem, problably by simply doing ... 

if( !response.ended() ) pump.stop();

[Francois Delisle]

May I suggest using something else than endHandler as the name since response.end( ..) and response.ended() have a very specific meaning: the response has been written successfully.

Instead, why not call it the completionHandler and add a method .completed().

A use case for using completed() would be when returning from blocking code that was executing asynchronously. Say during that time the response completed because the underlying connection was closed by the client. In the resultHandler invoked back on the execution context (event thread), first thing I would do would be to abort if the response.completed(). Right now I can use .ended(), but that doesn't cover the case I just described, and unfortunately response.closed still returns true in this case.

[Francois Delisle]

 and unfortunately response.closed still returns true in this case.

correction: still returns false in this case