On 30/10/2014 12:20, Rémy Maucherat wrote: > 2014-10-30 10:42 GMT+01:00 Mark Thomas <ma...@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 >> behaviour: >> - 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 > now). 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 either. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
