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.
Stuart
>
>
> regards
>
>
>
>
>
>
>
>
>
>
>
>
> --
> Greg Wilkins <gregw@webtide.com> CTO http://webtide.com