On 3/6/2012 1:36 AM, Remy Maucherat wrote:
> 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 replied to your email didn't I?
>
>>> 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.
So a onTimeout callback on the ReadListener and WriteListener would
work? Something
along the lines of
ReadListener
onTimeout(ServletRequest req)
WriteListener
onTimeout(ServletResponse res)
and change the addReadListener to setReadListener(ReadListener listener,
long timeout)
and change addWriteListener to setWriteListener(WriteListener listener,
long timeout)
where if timeout is 0 then it would never timeout.
Does that make sense?
- Rajiv
>
>>> - 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.
>