On 30/10/2014 12:20, Rémy Maucherat wrote:
2014-10-30 10:42 GMT+01:00 Mark Thomas <markt@apache.org>:
Was it just the cost of the chunking that triggered this change or was
it something else? If it was performance, how much difference does this
change make?
Debugging something else, this turned out to not cause a failure, it was
the reversed header order that caused the problem.
OK. Order can be significant for multiple instances of the same header
so it makes sense to ensure that order is retained.
Looking at the code, I can see a couple of places where this changes
- If the client requests the connection is closed
Yes, that was this situation, the client sends a connection close. But in
that case connection close is also added to the response (right after this
code) so the behavior is consistent.

- If the request is the last one before the connection is closed due to
reaching the maximum number of keep-alive requests

We have had bug reports in the past about not being able to detect a
truncated response. I suspect the current unit tests for those miss the
second case above which is my main concern right now.

I don't think a system property is the right way to handle configuration
of this feature (if such configuration is required). There is no reason
it could not be a connector attribute.
IMO the new attribute should also do the same thing for connection close in
the response. The default should be to not chunk if closing (as is right
I disagree on the default but not adding the behaviour. Users have had
issues (not being able to identify a truncated response) because we
haven't used chunked encoding but no user has reported an issue because
we have used chunked encoding. Therefore the default should be to
continue using chunked encoding with an option to disable it for those
that want the marginal performance gain it brings.
Ok, so since you're complaining about every commit,
No, I am not. I am complaining about you frequently breaking the unit
tests when I am trying to get the code base to a point where I can tag
it for the next release (i.e. the unit tests have to pass).

I am also not particularly happy with commits like this one that
knowingly introduce regressions for users in order to fix a perceived
problem that isn't actually causing an issue for anyone.
next fix you'll scream more since the upgrade process is not correct
If HTTP upgrade is broken, I'm not going to object to fixing it. If the
fix breaks some unit tests then I'm likely to moan about that.
(no handling of leftover input bytes, anything included in the same packet
that the request header is lost - I would agree it's questionable
client behavior though).
I've been digging through the code. I thought I recalled doing something
like this for WebSocket but it was on the client side where such
behaviour is expected.

I agree it is questionable client behaviour but I'm guessing you've seen
at least one client doing this. I can understand why they might.
This means adding code to pass around a ByteBuffer to the upgraded
processor to add as input since I see no other way :) Yum.
Given the implementation of HTTP upgrade (dedicated upgrade processors,
deliberately light-weight compared to the HTTP processors, none of the
I/O scaffolding the HTTP processors have) I don't see an alternative way


To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Search Discussions

Discussion Posts


Related Discussions

Discussion Navigation
viewthread | post
posts ‹ prev | 5 of 5 | next ›
Discussion Overview
groupdev @
postedOct 30, '14 at 8:43a
activeOct 30, '14 at 2:08p

2 users in discussion

Rémy Maucherat: 3 posts Mark Thomas: 2 posts



site design / logo © 2018 Grokbase