On 12/01/18 20:58, Christopher Schultz wrote: > Mark, > > On 1/12/18 3:27 AM, Mark Thomas wrote: >> On 11/01/18 23:12, Christopher Schultz wrote: > >> <snip/> > >>> If performance is a consideration, then most of the calls to >>> write() should probably be calls to headerBuffer.put() because we >>> can be (reasonably?) sure that writing "HTTP/1.1 " to the output >>> buffer isn't going to overflow the buffer. Likewise with the >>> bytes for the status code. With an omitted reason phrase, even >>> the trailing SP CR LF can be collapsed into a single byte[] and >>> written a single time to the ByteBuffer, instead of individually >>> as the current code does. >>> >>> If there is a reason to be inconsistent, let's state it and be >>> consistently inconsistent. If there is no such reason, let's be >>> consistent and either always call write() or always call >>> headerBuffer.put(). >>> >>> Thoughts? > >> The length check in write includes an allowance for an additional >> 4 bytes. See BZ 57509 for background. > >> I agree that the additional checks (via write()) in sendStatus() >> are unnecessary. If a user sets maxHttpHeaderSize so low you can't >> write the status line then a BufferOverflowException seems >> perfectly reasonable. > >> I'm in favour of switching sendStatus() to use headerBuffer.put() >> throughout, along with adding a comment to the effect that the >> buffer is reset for each request so, unless maxHttpHeaderSize is >> set so low nothing works, the status line will always fit so write >> it directly without length checks. > > What do you think about a try/catch for BufferOverflowException that > re-throws the exception as HeadersTooLargeException? Would that > try/catch negatively impact performance enough to earn a -1 ? Since it > would be a hit exactly once per response (right?) it shouldn't be a > big deal, especially when offset by the savings of not re-checking the > buffer many times over (which is my proposal, here, of course).
I'm not -1 on a try/catch but I strongly prefer not having it. It the user is daft enough to set maxHttpHeaderSize to less than the length of: HTTP/1.1 nnn CRLF then the deserve all they get 0 including an ugly exception. I'd prefer the cleaner code. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
