On Mon, 2012-03-05 at 13:55 -0800, Rajiv Mordani wrote:
> Remy,
> See comments in-line -
>
> On 02/29/2012 01:17 AM, Remy Maucherat wrote:
> > On Tue, 2012-02-28 at 11:19 -0800, Shing Wai Chan wrote:
> >> I have enclosed the candidate Servlet 3.1 Early draft review with change
> >> bars and the associated javadoc.
> >> The source code can be found at
> >> https://svn.java.net/svn/glassfish~svn/trunk/api/javaee-api/javax.servlet .
> >> For a list of changes, please look at the status chapter.
> >> Please give us feedback by Mar 6 (next Tue).
> > Some feedback focused exclusively on the upgraded connection section,
> > since it is brand new.
>
> I will let Shing Wai answer the upgrade part of the questions since he
> was driving most of
> it.
My feedback was exclusively on the upgrade part. I already sent you some
feedback on the rest (but didn't get an answer).
> > I understand the reasons for the addition of the upgraded connections
> > and I am now ok with the concept, but I have reservations about how they
> > are specified at the moment.
> >
> > - Half closes. What is the use of that exactly ?
Could I get an answer on that item ?
> > - I don't think using ServletIS and ServletOS in this API is a good
> > idea. In particular, ServletOS.flush(), ServletIS.readLine() and the
> > ServletOS.write(byte[]) with its blocking semantics (the full array must
> > be written) all seem to be a problem. I think read should instead
> > provide only one method: read(ByteBuffer) (and write would have
> > write(ByteBuffer)). With both being allowed to read or write 0 bytes,
> > probably. Or something like that.
>
> I assume that at this point you aren't referring to the upgrade API :).
No ... WebConnection is using ServletIS and ServletOS, which don't make
sense.
> > - Using Read/WriteListener is not optional in this part, unlike for
> > regular Servlet 3.1, which means blocking IO is not available. I'm
> > skeptical about the write notification, which has the potential of being
> > called all the time.
>
> I think I clarified this in the spec but if it isn't clear then I can
> attempt to do it again. Just like the
> ReadListener the WriteListener will be called only once till a write
> operation is carried out and then
> will be notified again only after that.
Ok.
> > - No notifications of timeouts. Everything is considered an error, with
> > a throwable included, and separated between input and output. That seems
> > very odd.
>
> Not clear what the time out would be for? Can you clarify more?
If a connection is idle for a while, it should trigger a timeout.
Although keeping a socket alive is not that expensive, it is still a
restricted resource in the OS. It is similar to the timeout support in
Servlet 3.0 async.
> > - No provision for sleeping and resuming beyond IO triggered events,
> > like the Servlet 3.0 async capability is providing. Protocol handler
> > could include a resume(WebConnection) and WebConnection could have a
> > suspend (or sleep) method. Although this could be done by the protocol
> > handlers, there could be concurrency issues between the thread pool it
> > would use and the IO operations done by the container. So adding the
> > feature would allow the container to provide full thread management like
> > it does for Servlet 3.0 and 3.1.
So what do you think about that last point ? Upgraded connections could
use the more flexible thread management.
--
Remy Maucherat <rmaucher_at_redhat.com>
Red Hat Inc