FAQ

[Tomcat-users] Connection is closed when CometEvent.close is called during an event

Reich, Matthias
Jun 8, 2010 at 10:26 am
Hello,

I am using a CometProcessor servlet in a long-poll scenario, and recently had
a closer look at the life span of connections that are used for poll requests.

I noticed that connections are closed by Tomcat whenever a poll request
was answered (and closed) directly during processing of the BEGIN event.
In our application this happens for one out of three poll requests approximately
and thus should not be neglected.

I had a look into the source code and found the reason in the
CoyoteAdapter.event method - it sets the 'error' flag to true in this situation.

I modified the code (in 6.0.20 and 6.0.26) so that the error flag is not set.
With that change Tomcat kept the connection open:

...
// Calling the container
connector.getContainer().getPipeline().getFirst().event(request, response, request.getEvent());

if (!error && !response.isClosed() && (request.getAttribute(Globals.EXCEPTION_ATTR) != null)) {
// An unexpected exception occurred while processing the event, so
// error should be called
request.getEvent().setEventType(CometEvent.EventType.ERROR);
request.getEvent().setEventSubType(null);
error = true;
connector.getContainer().getPipeline().getFirst().event(request, response, request.getEvent());
}
if (response.isClosed() || !request.isComet()) {
if (status==SocketStatus.OPEN) {
//CometEvent.close was called during an event.
request.getEvent().setEventType(CometEvent.EventType.END);
request.getEvent().setEventSubType(null);

// don't set the error flag here - otherwise the connection will be closed
// whenever a long poll is answered already during event handling:
// error = true;

connector.getContainer().getPipeline().getFirst().event(request, response, request.getEvent());
}
res.action(ActionCode.ACTION_COMET_END, null);
} else if (!error && read && request.getAvailable()) {
// If this was a read and not all bytes have been read, or if no data
// was read from the connector, then it is an error
request.getEvent().setEventType(CometEvent.EventType.ERROR);
request.getEvent().setEventSubType(CometEvent.EventSubType.IOEXCEPTION);
error = true;
connector.getContainer().getPipeline().getFirst().event(request, response, request.getEvent());
}
return (!error);
...

In my first tests I did not observe any undesired side effects of the change.
However, I did not yet do extensive tests - especially not with a steaming client.

Do you agree that this should be considered a bug and fixed in the next Tomcat
release?


Regards,
Matthias


---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
reply

Search Discussions

6 responses

  • Pid at Jun 10, 2010 at 9:27 am

    On 08/06/2010 11:26, Reich, Matthias wrote:
    Hello,

    I am using a CometProcessor servlet in a long-poll scenario, and recently had
    a closer look at the life span of connections that are used for poll requests.

    I noticed that connections are closed by Tomcat whenever a poll request
    was answered (and closed) directly during processing of the BEGIN event.
    In our application this happens for one out of three poll requests approximately
    and thus should not be neglected.

    I had a look into the source code and found the reason in the
    CoyoteAdapter.event method - it sets the 'error' flag to true in this situation.

    I modified the code (in 6.0.20 and 6.0.26) so that the error flag is not set.
    With that change Tomcat kept the connection open:

    ...
    // Calling the container
    connector.getContainer().getPipeline().getFirst().event(request, response, request.getEvent());

    if (!error && !response.isClosed() && (request.getAttribute(Globals.EXCEPTION_ATTR) != null)) {
    // An unexpected exception occurred while processing the event, so
    // error should be called
    request.getEvent().setEventType(CometEvent.EventType.ERROR);
    request.getEvent().setEventSubType(null);
    error = true;
    connector.getContainer().getPipeline().getFirst().event(request, response, request.getEvent());
    }
    if (response.isClosed() || !request.isComet()) {
    if (status==SocketStatus.OPEN) {
    //CometEvent.close was called during an event.
    request.getEvent().setEventType(CometEvent.EventType.END);
    request.getEvent().setEventSubType(null);

    // don't set the error flag here - otherwise the connection will be closed
    // whenever a long poll is answered already during event handling:
    // error = true;

    connector.getContainer().getPipeline().getFirst().event(request, response, request.getEvent());
    }
    res.action(ActionCode.ACTION_COMET_END, null);
    } else if (!error && read && request.getAvailable()) {
    // If this was a read and not all bytes have been read, or if no data
    // was read from the connector, then it is an error
    request.getEvent().setEventType(CometEvent.EventType.ERROR);
    request.getEvent().setEventSubType(CometEvent.EventSubType.IOEXCEPTION);
    error = true;
    connector.getContainer().getPipeline().getFirst().event(request, response, request.getEvent());
    }
    return (!error);
    ...

    In my first tests I did not observe any undesired side effects of the change.
    However, I did not yet do extensive tests - especially not with a steaming client.

    Do you agree that this should be considered a bug and fixed in the next Tomcat
    release?
    You'll need one of the devs with Async knowledge to look at this, I think.


    p
    Regards,
    Matthias


    ---------------------------------------------------------------------
    To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
    For additional commands, e-mail: users-help@tomcat.apache.org
  • Konstantin Kolinko at Jun 10, 2010 at 7:49 pm

    2010/6/8 Reich, Matthias <matthias.reich@siemens-enterprise.com>:
    I modified the code (in 6.0.20 and 6.0.26) so that the error flag is not set.
    With that change Tomcat kept the connection open:

    ...
    if (response.isClosed() || !request.isComet()) {
    if (status==SocketStatus.OPEN) {
    //CometEvent.close was called during an event.
    request.getEvent().setEventType(CometEvent.EventType.END);
    request.getEvent().setEventSubType(null);

    // don't set the error flag here - otherwise the connection will be closed
    // whenever a long poll is answered already during event handling:
    // error = true;
    1. I think that I do not understand you. What is your meaning of "long
    poll"? Can you describe your situation as a sequence of events how
    they occur step-by-step on a timeline?

    2. The above fragment when using Comet should be equivalent to
    if (response.isClosed()) {
    if (status==SocketStatus.OPEN) {
    //CometEvent.close was called during an event.
    request.getEvent().setEventType(CometEvent.EventType.END);
    request.getEvent().setEventSubType(null);

    // don't set the error flag here - otherwise the connection will be closed
    // whenever a long poll is answered already during event handling:
    // error = true;
    Response#isClosed():
    public boolean isClosed() {
    return outputBuffer.isClosed();
    }

    If you will not be able to send your answer, why not to close the
    socket right away?

    3. It would be much more readable, if you provided your changes in the
    unified diff format. (even better if it were generated with "svn diff"
    command against sources retrieved from svn).


    Best regards,
    Konstantin Kolinko

    ---------------------------------------------------------------------
    To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
    For additional commands, e-mail: users-help@tomcat.apache.org
  • Reich, Matthias at Jun 10, 2010 at 11:35 pm

    On 10/06/2010 9:49 PM, Konstantin Kolinko wrote:
    2010/6/8 Reich, Matthias:
    I modified the code (in 6.0.20 and 6.0.26) so that the
    error flag is not set.
    With that change Tomcat kept the connection open:

    ...
    if (response.isClosed() || !request.isComet()) {
    if (status==SocketStatus.OPEN) {
    //CometEvent.close was called during an event.
    request.getEvent().setEventType(CometEvent.EventType.END);
    request.getEvent().setEventSubType(null);

    // don't set the error flag here - otherwise the connection
    will be closed
    // whenever a long poll is answered already during event handling:
    // error = true;
    1. I think that I do not understand you. What is your meaning of "long
    poll"? Can you describe your situation as a sequence of events how
    they occur step-by-step on a timeline?
    The concept of long poll is e.g. described in
    http://www.javaworld.com/javaworld/jw-03-2008/jw-03-asynchhttp.html?page=6

    The sequence of events in my situation is as follows:
    - a poll request is received by the server
    - the CoyoteAdapter.service method is called and in turn
    invokes the servlet's event method with a BEGIN event
    - request.isComet() is still true when the control returns
    to the CoyoteAdapter.service method
    - some other thread writes a response and closes the Writer
    of the response
    - the CoyoteAdapter.event method is called and in turn
    invokes the servlet's event method with an END event
    - the servlet calls event.close()
    - when the control returns to the CoyoteAdapter.event method
    we have exactly this situation:
    response.isClosed() && !request.isComet() && status==SocketStatus.OPEN
    - thus, if the error flag is set in this situation,
    the connection will be closed, and a new connection must be opened
    by the browser for the subsequent poll request

    According to the above sequence I would expect that the connection
    is always closed if request.isComet() is still true when control
    returns to the CoyoteAdapter.service method after processing
    the BEGIN event - no matter how long it takes from then
    until the response is written.
    Surprisingly, I did not always observe this.

    Anyway, if the error flag is not set in this situation,
    the connection is kept open.

    2. The above fragment when using Comet should be equivalent to
    if (response.isClosed()) {
    if (status==SocketStatus.OPEN) {
    //CometEvent.close was called during an event.
    request.getEvent().setEventType(CometEvent.EventType.END);
    request.getEvent().setEventSubType(null);

    // don't set the error flag here - otherwise the connection
    will be closed
    // whenever a long poll is answered already during event handling:
    // error = true;
    Response#isClosed():
    public boolean isClosed() {
    return outputBuffer.isClosed();
    }
    No, it is not equivalent: response.isClosed() is true after closing
    the Writer or OutputStream, whereas request.isComet() is true
    until event.close() is called.
    If you will not be able to send your answer, why not to close the
    socket right away?
    I was able to send the answer and would like to use the connection
    also for the next poll request. (or for some other request
    the browser decides to send through this connection)
    3. It would be much more readable, if you provided your changes in the
    unified diff format. (even better if it were generated with "svn diff"
    command against sources retrieved from svn).
    --- C:\DOCUME~1\rm041693\LOCALS~1\Temp\CoyoteAdapter.java-revBASE.svn001.tmp.java Do Jun 10 22:22:20 2010
    +++ D:\tomcat\TOMCAT_6_0_26\java\org\apache\catalina\connector\CoyoteAdapter.java Mo Jun 7 17:30:23 2010
    @@ -215,7 +215,9 @@
    //CometEvent.close was called during an event.
    request.getEvent().setEventType(CometEvent.EventType.END);
    request.getEvent().setEventSubType(null);
    - error = true;
    + // don't set the error flag - otherwise the socket will be closed
    + // whenever CometEvent.close is called during the event
    + // error = true;
    connector.getContainer().getPipeline().getFirst().event(request, response, request.getEvent());
    }
    res.action(ActionCode.ACTION_COMET_END, null);
    Best regards,
    Konstantin Kolinko
    Regards,
    Matthias

    ---------------------------------------------------------------------
    To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
    For additional commands, e-mail: users-help@tomcat.apache.org
  • Konstantin Kolinko at Jun 11, 2010 at 2:46 am
    2010/6/11 Reich, Matthias <matthias.reich@siemens-enterprise.com>:
    The concept of long poll is e.g. described in
    http://www.javaworld.com/javaworld/jw-03-2008/jw-03-asynchhttp.html?page=6

    The sequence of events in my situation is as follows:
    - a poll request is received by the server
    - the CoyoteAdapter.service method is called and in turn
    invokes the servlet's event method with a BEGIN event
    - request.isComet() is still true when the control returns
    to the CoyoteAdapter.service method
    - some other thread writes a response and closes the Writer
    of the response
    - the CoyoteAdapter.event method is called and in turn
    invokes the servlet's event method with an END event
    - the servlet calls event.close()
    - when the control returns to the CoyoteAdapter.event method
    we have exactly this situation:
    response.isClosed() && !request.isComet() && status==SocketStatus.OPEN
    - thus, if the error flag is set in this situation,
    the connection will be closed, and a new connection must be opened
    by the browser for the subsequent poll request

    According to the above sequence I would expect that the connection
    is always closed if request.isComet() is still true when control
    returns to the CoyoteAdapter.service method after processing
    the BEGIN event -  no matter how long it takes from then
    until the response is written.
    Surprisingly, I did not always observe this.

    Anyway, if the error flag is not set in this situation,
    the connection is kept open.

    2. The above fragment when using Comet should be equivalent to
    if (response.isClosed()) {
    if (status==SocketStatus.OPEN) {
    //CometEvent.close was called during an event.
    request.getEvent().setEventType(CometEvent.EventType.END);
    request.getEvent().setEventSubType(null);

    // don't set the error flag here - otherwise the connection
    will be closed
    // whenever a long poll is answered already during event handling:
    // error = true;
    Response#isClosed():
    public boolean isClosed() {
    return outputBuffer.isClosed();
    }
    No, it is not equivalent: response.isClosed() is true after closing
    the Writer or OutputStream, whereas request.isComet() is true
    until event.close() is called.
    If you will not be able to send your answer, why not to close the
    socket right away?
    I was able to send the answer and would like to use the connection
    also for the next poll request. (or for some other request
    the browser decides to send through this connection)
    3. It would be much more readable, if you provided your changes in the
    unified diff format. (even better if it were generated with "svn diff"
    command against sources retrieved from svn).
    --- C:\DOCUME~1\rm041693\LOCALS~1\Temp\CoyoteAdapter.java-revBASE.svn001.tmp.java       Do Jun 10 22:22:20 2010
    +++ D:\tomcat\TOMCAT_6_0_26\java\org\apache\catalina\connector\CoyoteAdapter.java       Mo Jun  7 17:30:23 2010
    @@ -215,7 +215,9 @@
    //CometEvent.close was called during an event.
    request.getEvent().setEventType(CometEvent.EventType.END);
    request.getEvent().setEventSubType(null);
    -                        error = true;
    +                        // don't set the error flag - otherwise the socket will be closed
    +                        // whenever CometEvent.close is called during the event
    +                        // error = true;
    connector.getContainer().getPipeline().getFirst().event(request, response, request.getEvent());
    }
    res.action(ActionCode.ACTION_COMET_END, null);
    Now I understand. Thank you.

    I would say that you are trying to combine Comet and Keep-Alive.

    In comet to send a portion of data (a response), you do
    writer.flush(). It sends the data over the wire. Doing event.close()
    terminates comet request processing.

    If it were possible not to close the connection, it were possible to
    alternate comet and non-comet processing of subsequent requests over
    the same connection, and to process different requests by different
    servlets.
    - some other thread writes a response and closes the Writer
    of the response
    Response object is not thread safe. You must write your response in
    the thread that received your EventType.READ event (or any other
    event) while you are processing that event.

    Otherwise, any random result might happen.


    BTW, for reference:
    There exists the following documentation page,
    http://tomcat.apache.org/tomcat-6.0-doc/aio.html#CometEvent
    and sample code in
    webapps/examples/WEB-INF/classes/chat/ChatServlet.java
    plus some helper JSPs.

    The sample is callable as
    http://localost:8080/examples/jsp/chat/chat.jsp in Tomcat 6.0.26 and earlier
    http://localost:8080/examples/jsp/chat/index.jsp in Tomcat 7 and in
    Tomcat 6.0.27 and later


    Regarding Comet + Keep-Alive, if it does not work, it is worth filing
    an enhancement request against Tomcat 7. It would be easier if there
    were some sample code or better a test case. This new use case has to
    be tested.

    The patch you proposing looks promising, but as of now I am not sure that

    a) both cases when that branch is called are equivalent,
    I mean (response.isClosed()) vs. (!request.isComet()) whether in both
    cases we can go without closing the socket
    Calling CometEventImpl.close() will set both conditions to true.
    I think that response.isClosed() when isComet() == true will mean an
    error and needs closing the socket. Need to think more about it.

    b) how the socket will be processed further and be returned to the
    poller. There are non-comet vs. comet pollers. There are some
    keepAlive settings (like "maxKeepAliveRequests") and those have to be
    respected.


    Best regards,
    Konstantin Kolinko

    ---------------------------------------------------------------------
    To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
    For additional commands, e-mail: users-help@tomcat.apache.org
  • Reich, Matthias at Jun 15, 2010 at 8:13 am

    Konstantin Kolinko wrote:
    2010/6/11 Reich, Matthias <matthias.reich@siemens-enterprise.com>:
    The concept of long poll is e.g. described in
    http://www.javaworld.com/javaworld/jw-03-2008/jw-03-asynchhttp
    .html?page=6
    The sequence of events in my situation is as follows:
    - a poll request is received by the server
    - the CoyoteAdapter.service method is called and in turn
    invokes the servlet's event method with a BEGIN event
    - request.isComet() is still true when the control returns
    to the CoyoteAdapter.service method
    - some other thread writes a response and closes the Writer
    of the response
    - the CoyoteAdapter.event method is called and in turn
    invokes the servlet's event method with an END event
    - the servlet calls event.close()
    - when the control returns to the CoyoteAdapter.event method
    we have exactly this situation:
    response.isClosed() && !request.isComet() &&
    status==SocketStatus.OPEN
    - thus, if the error flag is set in this situation,
    the connection will be closed, and a new connection must be opened
    by the browser for the subsequent poll request

    According to the above sequence I would expect that the connection
    is always closed if request.isComet() is still true when control
    returns to the CoyoteAdapter.service method after processing
    the BEGIN event -  no matter how long it takes from then
    until the response is written.
    Surprisingly, I did not always observe this.

    Anyway, if the error flag is not set in this situation,
    the connection is kept open.



    ---
    C:\DOCUME~1\rm041693\LOCALS~1\Temp\CoyoteAdapter.java-revBASE.
    svn001.tmp.java       Do Jun 10 22:22:20 2010
    +++
    D:\tomcat\TOMCAT_6_0_26\java\org\apache\catalina\connector\Coy
    oteAdapter.java       Mo Jun  7 17:30:23 2010
    @@ -215,7 +215,9 @@
    //CometEvent.close was called
    during an event.

    request.getEvent().setEventType(CometEvent.EventType.END);
    request.getEvent().setEventSubType(null);
    -                        error = true;
    +                        // don't set the error flag -
    otherwise the socket will be closed
    +                        // whenever CometEvent.close is
    called during the event
    +                        // error = true;
    connector.getContainer().getPipeline().getFirst().event(reques
    t, response, request.getEvent());
    }
    res.action(ActionCode.ACTION_COMET_END, null);
    Now I understand. Thank you.

    I would say that you are trying to combine Comet and Keep-Alive.
    Well, it actually was combined (maybe unintentionally) in 6.0.18.
    (I retested an older version of our application which uses 6.0.18
    and didn't observe that connections were closed in similar situations.)

    Since 6.0.20, the connections are closed upon event.close(),
    but it took me some time to get aware of this because when the
    subsequent request fails due to the close, the browsers
    silently retry that request.
    In comet to send a portion of data (a response), you do
    writer.flush(). It sends the data over the wire. Doing event.close()
    terminates comet request processing.

    If it were possible not to close the connection, it were possible to
    alternate comet and non-comet processing of subsequent requests over
    the same connection, and to process different requests by different
    servlets.
    - some other thread writes a response and closes the Writer
    of the response
    Response object is not thread safe. You must write your response in
    the thread that received your EventType.READ event (or any other
    event) while you are processing that event.

    Otherwise, any random result might happen.
    Doesn't the sample code on the documentation page include async
    writes as well? When it gets a READ event from one connection
    it writes to all open connetions.
    With respect to all other connections, this is an async write.

    In a long poll scenario, closing the Writer seems to be
    the only way to trigger a further event (i.e. an END event)
    after the BEGIN event.
    BTW, for reference:
    There exists the following documentation page,
    http://tomcat.apache.org/tomcat-6.0-doc/aio.html#CometEvent
    and sample code in
    webapps/examples/WEB-INF/classes/chat/ChatServlet.java
    plus some helper JSPs.

    The sample is callable as
    http://localost:8080/examples/jsp/chat/chat.jsp in Tomcat
    6.0.26 and earlier
    http://localost:8080/examples/jsp/chat/index.jsp in Tomcat 7 and in
    Tomcat 6.0.27 and later


    Regarding Comet + Keep-Alive, if it does not work, it is worth filing
    an enhancement request against Tomcat 7. It would be easier if there
    were some sample code or better a test case. This new use case has to
    be tested.
    I'll try to find the time to prepare something. In the repository
    (6.0.26) I have not detected any Comet related test cases. Are there
    any test cases you could recommend as an example of how I should
    organize it?
    What is the best way to deliver the code to you?
    The patch you proposing looks promising, but as of now I am
    not sure that

    a) both cases when that branch is called are equivalent,
    I mean (response.isClosed()) vs. (!request.isComet()) whether in both
    cases we can go without closing the socket
    Calling CometEventImpl.close() will set both conditions to true.
    I think that response.isClosed() when isComet() == true will mean an
    error and needs closing the socket. Need to think more about it.
    I think you are right.
    If the response is already closed before the container is invoked,
    the servlet receives an END event and must call event.close().
    If it doesn't, it is an error.

    However, the following CAN happen:
    - the servlet processes a READ event and therefore
    does not call event.close()
    - then the response is closed by another thread
    - then control returns to CoyoteAdpater.event
    - now we have response.isClosed() when isComet() == true

    This can only happen in the streaming case, as it requires a
    READ event, and even if it is not really an error, I think
    it is acceptable to close the socket in this situation.


    The servlet programmer knows if the servlet implements
    long poll or streaming.
    So, maybe the servlet should be enabled to declare
    that the connection shall be kept open, e.g. via an
    additional method CometEvent.close(boolean keepalive).
    Existing code that relies on the socket close would not be
    broken if CometEvent.close() would behave like
    CometEvent.close(false).

    Such an interface extension would also have the charm
    of making explicit that closing an event does not only affect
    the associated request but also the connection.
    b) how the socket will be processed further and be returned to the
    poller. There are non-comet vs. comet pollers. There are some
    keepAlive settings (like "maxKeepAliveRequests") and those have to be
    respected.
    For long poll I want to keep the polling connection open
    as long as possible. Therefore I am using maxKeepAliveRequests = -1.


    Best regards,
    Matthias Reich


    ---------------------------------------------------------------------
    To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
    For additional commands, e-mail: users-help@tomcat.apache.org
  • Reich, Matthias at Jun 29, 2010 at 8:16 am

    -----Original Message-----
    From: Konstantin Kolinko
    Sent: Friday, June 11, 2010 4:46 AM
    To: Tomcat Users List
    Subject: Re: Connection is closed when CometEvent.close is
    called during an event
    Regarding Comet + Keep-Alive, if it does not work, it is worth filing
    an enhancement request against Tomcat 7. It would be easier if there
    were some sample code or better a test case. This new use case has to
    be tested.
    I have had a look at Tomcat 7 and recognized that with Tomcat 7 I would
    probably refactor my application to use Servlet 3 conforming
    asynchronous processing instead of the CometProcessor interface.

    I assume that keep alive will be supported in conjunction with async
    processing?
    Then, it would no longer make sense using the CometProcessor interface
    to implement long poll.

    However, I am not sure if it will be possible for us to change
    to Tomcat 7 soon. From the release notes I read that it requires Java 6
    and I suspect that we will be bound to Java 5 for quite a while.

    Hence, is there also any interest in respective improvements for Tomcat 6?
    Then I could provide an example servlet as a patch.


    Regards,
    Matthias




    ---------------------------------------------------------------------
    To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
    For additional commands, e-mail: users-help@tomcat.apache.org

Related Discussions

Discussion Navigation
viewthread | post