Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

on_finish() is not called when the client closed first. #517

KevinHua opened this Issue May 25, 2012 · 6 comments


None yet
3 participants

class Webbench(tornado.web.RequestHandler):
count = 0
fini_count = 0;
closed_count = 0;

def get(self, *args, **kwargs):
    Webbench.count += 1
    logging.debug("Webbench client count %d" % Webbench.count)

    self.write("hello world " + str(Webbench.count))
    // the finish() cause on_finish() being called.
    // self.finish()

def on_finish(self):
    Webbench.fini_count += 1
    logging.debug("Webbench finished %d, closed %d, total %d" 
                  % (Webbench.fini_count, Webbench.closed_count, Webbench.count))

def on_connection_close(self):
    // when client reset or closed connection, the on_finish() was not called.

    Webbench.closed_count += 1
    logging.debug("Webbench closed %d, finished %d, total %d" 
                  % (Webbench.closed_count, Webbench.fini_count, Webbench.count))

bdarnell commented May 26, 2012

This is as intended - you should get one of on_finish or on_connection_close, but not both. Or are you seeing a case where neither gets called?

The doc string of on_finish() is said it's the place to release resource such as database connection, so I wondered that it would lead to resource leak since on_finish() won't be called when client is closed or reset before the server finish() the request. I saw this case when using apache ab to benchmark my app.

Of couse, one of the two will be called in any circumstance. And I have added a call to on_finish() in my on_connection_close to avoid potential resource leaks. But I was still misunderstood of the disaccording between the doc strings of on_finish() and on_connection_close(). Maybe, a same tip should be added to on_connection_close(), I think.


E-mail: huahl@126.com
MSN: kvhua@msn.com

At 2012-05-27 02:20:43,bdarnell reply@reply.github.com wrote:

This is as intended - you should get one of on_finish or on_connection_close, but not both. Or are you seeing a case where neither gets called?

Reply to this email directly or view it on GitHub:
facebook#517 (comment)


bdarnell commented May 29, 2012

That's a fair point; this could be documented better. When on_finish was introduced (in 2.2), the previous recommendation was to override finish() (and be sure to call the superclass version), so on_finish() is called exactly when finish() is.

Actually, now that I think about it it's not true that only one of on_connection_close and finish will be called. If the connection is closed then on_connection_close will be called, but if that doesn't cause pending operations to be cancelled then the request handler will continue on and eventually finish. I think finish() will currently raise an exception if you reach this point so it won't call on_finish. We need a more consistent policy about how a request should be aborted when the connection is closed.

@bdarnell bdarnell added the web label Jul 16, 2014

Has this behaviour been changed recently? Or do I still need to do the workaround where on_finish is called from on_connection_close as described above?

The documentation is really not clear on when on_close is called, and that it is specific to the client closing the connection. It simply says:

Invoked when the WebSocket is closed.

I think this needs clarification.


bdarnell commented May 20, 2015

Nothing has changed recently. For regular RequestHandlers: if you terminate request processing in on_connection_close, on_finish will not be called for you. The framework cannot do this for you because it cannot know whether you terminate processing immediately, set a flag to terminate it later, or allow it to continue.

For WebSocketHandlers, on_close() is called no matter which side closed the connection. In tornado 4.1 and earlier on_close() could sometimes be called twice, but in 4.2 it will be called exactly once.

Ok thank you for a fast reply. That makes things clearer :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment