Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [servlet-dev] Non-blocking IO and ServletOutputStream.close() 
On 10/01/2023 04:41, Greg Wilkins wrote:

Mark,
Also relevant is the 2019 discussion in https://github.com/jakartaee/servlet/issues/273 <https://github.com/jakartaee/servlet/issues/273> where I was again making the case that complete was a write and should wait for an isReady()==true before being legal to be called.
You can see that until that point, Jetty was implemented to treat a close or complete with a write pending as an error.   After that discussion and in response to user reported issues, jetty changed <https://github.com/eclipse/jetty.project/issues/4331> in Dec 2019 to follow undertow's model of allowing close and complete to be called with async operations pending.   These changes were also closely associated with a rethink/reimplement of sendError, which equally became a non-blocking state change method rather than a potentially blocking invocation of an error page dispatch.
It was a difficult reimplementation.... so perhaps a touch of I-don't-want-to-change-it-back bias now makes me think that the state change model is not so bad.
I can see the benefits of the state change model from the user perspective as well as the problems for users any attempt switch back would cause.
I've been looking into the history of the Tomcat implementation for non-blocking IO and, despite the comments in the test code and the discussion in the EG, Tomcat has always implemented the state change model for non-blocking IO. For close, Tomcat took a blocking approach.
It looks to be fairly simple to switch Tomcat to the state change approach for close. I have most of it implemented already.
My primary concern at this point is to make sure that the results of this discussion are reflected in the Servlet spec document and the Javadoc. Hopefully this will remove a large chunk of the ambiguity in the spec around async and non-blocking IO.
I'm still intended to create an issue with accompanying PR in the next few days (assuming the consensus to switch to the state change approach holds). 

Mark




cheers
On Tue, 10 Jan 2023 at 03:01, Mark Thomas <markt@xxxxxxxxxx <mailto:markt@xxxxxxxxxx>> wrote: 

    On 09/01/2023 09:04, Mark Thomas wrote:
     > On 07/01/2023 06:57, Greg Wilkins wrote:
     >> Mark,
     >>
     >> Conceptually both close and complete could be thought of as write
     >> operations that need to wait until a true isReady() return before
     >> being called.
     >>
     >> However, unlike a write operation, they cannot carry additional
    data
     >> to be queued, nor be called multiple times, so there is no
    unbounded
     >> data commitment if we let them be called whilst a write
    completion is
     >> still pending. In this interpretation, they are state changes
    rather
     >> than writes.
     >>
     >> So since the spec has never said otherwise, i think we must use the
     >> second interpretation.
     >
     > Thanks for the review Greg. There is a comment in the Tomcat code
    to the
     > effect that complete()/dispatch() are not allowed while async IO
    is in
     > progress. I need to do some research to see if I can find where that
     > comment originated as that point is rather fundamental to this
    discussion.

    The comment in the Tomcat code is [1]. It was added by Filip when he
    wrote the original test [2]. That is consistent with when the Servlet
    3.1 API was being discussed as part of JSR 340.

    I went back through the JSR 340 EG discussions to see if I could find
    anything that could have led to Filip adding that comment.

    The initial draft for async IO included a write() followed immediately
    by a complete() [3].

    When I asked what looks like the same question almost a decade ago, the
    consensus was that calling write() or complete() with a write in
    progress should trigger an exception [4].

    The topic was raised again in Servlet 4.0 / JSR 369 [5] and the
    conclusion was the same - calling complete() with a write in progress
    should trigger an exception.

    I haven't found any further discussions since until this one.

    I don't think any of the conclusions from those discussions made it
    into
    the spec document or the Javadoc.

    The TCK includes a test that calls complete() without isReady()
    returning true [6] and a test that calls write(), flush() and
    complete()
    in sequence [7].

    I am unable to explain why the behavior expected by the TCK is the
    exact
    opposite of what was agreed multiple times by the EG. I will note that
    the EG at the time, unlike now, had no access to the TCK source nor any
    ability to update the spec document or Javadoc.

    Given the behavior expected by the TCK, I don't think we have any
    choice
    but to allow close(), complete() and dispatch() while async read/writes
    are in progress.

    I'll leave this thread for a couple of days in case anyone has any
    further comments. Assuming no further discussion is required, I'll open
    an issue and provide a PR to update the Javadoc and spec document
    towards the end of this week / beginning of next.

    Mark




    [1]
    https://github.com/apache/tomcat/blame/main/test/org/apache/catalina/nonblocking/TestNonBlockingAPI.java#L823 <https://github.com/apache/tomcat/blame/main/test/org/apache/catalina/nonblocking/TestNonBlockingAPI.java#L823>

    [2]
    https://github.com/apache/tomcat/commit/690eb5c8d25d0fe26835386057bd455257cbdb86 <https://github.com/apache/tomcat/commit/690eb5c8d25d0fe26835386057bd455257cbdb86>

    [3]
    https://download.oracle.com/javaee-archive/servlet-spec.java.net/jsr340-experts/2011/09/0056.html <https://download.oracle.com/javaee-archive/servlet-spec.java.net/jsr340-experts/2011/09/0056.html>

    [4]
    https://download.oracle.com/javaee-archive/servlet-spec.java.net/jsr340-experts/2013/04/0569.html <https://download.oracle.com/javaee-archive/servlet-spec.java.net/jsr340-experts/2013/04/0569.html>

    [5]
    https://download.oracle.com/javaee-archive/servlet-spec.java.net/jsr369-experts/2015/11/0312.html <https://download.oracle.com/javaee-archive/servlet-spec.java.net/jsr369-experts/2015/11/0312.html>

    [6]
    https://github.com/jakartaee/platform-tck/blob/master/src/com/sun/ts/tests/servlet/api/jakarta_servlet_http/writelistener/TestListener.java <https://github.com/jakartaee/platform-tck/blob/master/src/com/sun/ts/tests/servlet/api/jakarta_servlet_http/writelistener/TestListener.java>

    [7]
    https://github.com/jakartaee/platform-tck/blob/master/src/com/sun/ts/tests/servlet/spec/async/TestServlet.java#L45 <https://github.com/jakartaee/platform-tck/blob/master/src/com/sun/ts/tests/servlet/spec/async/TestServlet.java#L45>
    _______________________________________________
    servlet-dev mailing list
    servlet-dev@xxxxxxxxxxx <mailto:servlet-dev@xxxxxxxxxxx>
    To unsubscribe from this list, visit
    https://www.eclipse.org/mailman/listinfo/servlet-dev
    <https://www.eclipse.org/mailman/listinfo/servlet-dev>



--
Greg Wilkins <gregw@xxxxxxxxxxx <mailto:gregw@xxxxxxxxxxx>> CTO http://webtide.com <http://webtide.com> 

_______________________________________________
servlet-dev mailing list
servlet-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/servlet-dev

Back to the top