jsr369-experts@servlet-spec.java.net

[jsr369-experts] Re: [servlet-spec users] Re: Re: Re: Re: revised trailer proposals

From: Martin Mulholland <mmulholl_at_us.ibm.com>
Date: Thu, 13 Apr 2017 13:59:04 -0400

Stuart Douglas <sdouglas_at_redhat.com> wrote on 04/12/2017 10:15:38 PM:

> From: Stuart Douglas <sdouglas_at_redhat.com>
> To: jsr369-experts_at_servlet-spec.java.net
> Date: 04/12/2017 10:16 PM
> Subject: [servlet-spec users] [jsr369-experts] Re: Re: Re: Re:
> revised trailer proposals
>
> On Thu, Apr 13, 2017 at 11:38 AM, Greg Wilkins <gregw_at_webtide.com>
wrote:
> >
> > On 13 April 2017 at 10:47, Shing Wai Chan <shing.wai.chan_at_oracle.com>
wrote:
> >>
> >> Look like we have an agreement on B.c (SuppierMap write).
> >
> >
> > \o/
> >
> >>
> >> Now, let us discuss more on A.c (RequestTrailerMap read) and A.e
> >> (ConsumerCallback read).
> >
> >
> >
> > I too am attracted to the symmetry of the ConsumerCallback API, but I
just
> > think that it requires very careful specification about who/when the
> > consumer is called.
> >
> > Consider a very simple request:
> >
> > GET / HTTP/1.1\r\n
> >
> > Transfer-Encoding: chunked\r\n
> >
> > \r\n
> >
> > 0\r\n
> >
> > Some-Trailer: value\r\n
> >
> > \r\n
> >
> > There is no content, so the application may never call
> > request.getInputStream().read(), so there is no opportunity to use the
> > caller of the read method to call the consumer. More over, it may be
that
> > the trailer is actually parsed before the servlet is even invoked.
> >
> > So if the trailer is already parsed before
> > setTrailerCallback(Consumer<Map<String, String>> callback) is called
and/or
> > read() is never called after it is called, then we do not have a
thread with
> > which we could call the callback. This problem is solvable, but only
by an
> > API contract that says the application must read the input to -1 after
the
> > callback is set, perhaps we should even through a ISE if the consumer
is set
> > after -1 is read? Perhaps something like:
>
> I would say that if the consumer is set after -1 is read then the
> callback is called immediately in the same thread that set the
> consumer.
>
> Otherwise you could have issues where some other spec or filter has
> caused the request to be parsed (e.g. by reading a parameter from a
> form encoded request) but you still want access to the trailers.
>
> >
> > /**
> >
> > * Set a Request Trailer Consumer
> > *
> >
> > * This method must be called before the application reads the
> >
> > * request content. After setting the consumer, the application must
> >
> > * read the content until EOF indication.
> >
> > *
> >
> > * @param callback Consumer of request headers which will be called
within
> >
> > * the scope of the call that reads the EOF indication of the request
> > content.
> > *
> >
> > * @exception IllegalStateException if the either of {_at_link
#getReader} or
> > * {_at_link #getInputStream} methods have already been called for this
request
> >
> > */
> > setTrailerCallback(Consumer<Map<String, String>> callback)
> >
> >
> > Now there are still some concerns with this. Let's say a client sends
the
> > request but stalls after an incomplete header. This means that a
blocking
> > read cannot progress and must block until the complete trailer is
received,
> > so it can call the consumer within the correct scope.
>
> That is no different to how things are now if the client stops sending
data?
>
> >
> > Similarly with async, we cannot call onAllDataRead until after the
entire
> > trailer is received. Which may lead to some funky logic around what
> > isReady() should return if we have seen the final chunk, but not the
full
> > trailer. We have to distinguish between parsing the end of the
content
> > and the end of the framing, which we had to do internally already, but
now
> > we are making that difference visible to the application.
>
> It should not really matter IMHO, you just delay readiness for the
> final read until all the trailers have been parsed.
>
> >
> > Again, there are all solvable, but subtle and if different containers
do
> > slightly different timings of how they consume trailers, then we may
have
> > portability problems. So it may be better to go with the simpler to
> > specify (but essentially equivalent):
> >
> > /**
> >
> > * Get a Request Trailer
> > *
> >
> > * This method can only be called after the application reads all
> >
> > * request content.
> >
> > *
> >
> > * @returns A map of trailers or null if the request did not contain
any.
> >
> > * @exception IllegalStateException if the neither
> > {_at_ReadListener#onAllDataRead}
> > * has been called nor an EOF indication has been
> >
> > * returned from the request {_at_link #getReader} or {_at_link
#getInputStream}
> >
> > */
> > Map<String,String> getTrailers()
>
> I am not against the simpler approach, I proposed the Consumer
> approach because it does have some advantages and more symmetry with
> the supplier approach (although also disadvantages, as you have
> pointed out).
>
> If everyone agrees I am happy with the getTrailers() approach, as it
> does seem much simpler and easier to specify.

I agree with the simpler approach. If we use a callback we would require
the servlet to support async in order to get trailer headers which seems
like
overkill.

Martin.

>
> Stuart
>
> >
> >
> > regards
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > --
> > Greg Wilkins <gregw@webtide.com> CTO http://webtide.com
>