Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Customizable error page buffer size #11654

Merged
merged 5 commits into from
Aug 29, 2024

Conversation

dkaukov
Copy link
Contributor

@dkaukov dkaukov commented Apr 15, 2024

Ability to increase buffer size past 8k in the custom ErrorHandler

@joakime
Copy link
Contributor

joakime commented Apr 16, 2024

The HttpConfiguration is how you control the output buffer size.

What is the use case for this?

@dkaukov
Copy link
Contributor Author

dkaukov commented Apr 16, 2024

True, but

    bufferSize = Math.min(8192, bufferSize); // TODO ?

limiting it to the 8k. We need a way to increase the limit.

@joakime
Copy link
Contributor

joakime commented Apr 17, 2024

limiting it to the 8k. We need a way to increase the limit.

I understand that you want to increase the buffer size limit, but why?
What's the use case for a larger buffer size? (I ask, as there's probably several different ways to accomplish your use case without increasing this specific buffer size)

@dkaukov
Copy link
Contributor Author

dkaukov commented Apr 18, 2024

Hi, In the

@Override
protected void writeErrorHtml(Request request, Writer writer, Charset charset, int code, String message, Throwable cause, boolean showStacks) throws IOException {
  ....
}

we are generating custom error page using Writer. Something like:

  themeLeafTemplate.execute(writer, errorInfo);

If error page is bigger than buffer size it is just trimmed.

@joakime
Copy link
Contributor

joakime commented Apr 18, 2024

Need to dig into why we are doing this via a static sized buffer.

I would like to see if this need can be accomplished via a Writer created with the Content.Sink features instead. (will try this in a different branch soon)

@dkaukov
Copy link
Contributor Author

dkaukov commented May 20, 2024

Hi @joakime , maybe we can merge this one in a meantime?

@gregw
Copy link
Contributor

gregw commented May 21, 2024

Need to dig into why we are doing this via a static sized buffer.

I believe the reasoning is that we don't want to get into complex responses when generating error pages, as if we are in an error state, we want simplicity.... otherwise we can get errors within errors within errors.....

I'll review this PR for merging now...

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is OK.... but I think I'd prefer a simple setBufferSize method, with a -1 meaning that a heuristic will be used (e.g. min(8K, httpConfig.bufferSize)). Default value will be -1 and public int getBufferSize() would return -1 (or the set value). Then have a protect int computeBufferSize(Request request) that would check for -1 and do the min(8k, config.bufferSize) thang.

protected int getBufferSize(Request request)
{
int bufferSize = request.getConnectionMetaData().getHttpConfiguration().getOutputBufferSize();
bufferSize = Math.min(8192, bufferSize); // TODO ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove the TODO from the code

@@ -262,6 +261,13 @@ else if (charsets.contains(StandardCharsets.ISO_8859_1))
}
}

protected int getBufferSize(Request request)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a bit of javadoc to say that this is getting the buffer size for the entire error response (any larger responses will be truncated). Then say that default implementation imposes an 8K limit on the standard buffer size, but that implementations may override the method to provide some other size.

@gregw
Copy link
Contributor

gregw commented May 21, 2024

This is OK.... but I think I'd prefer a simple setBufferSize method, with a -1 meaning that a heuristic will be used (e.g. min(8K, httpConfig.bufferSize)). Default value will be -1 and public int getBufferSize() would return -1 (or the set value). Then have a protect int computeBufferSize(Request request) that would check for -1 and do the min(8k, config.bufferSize) thang.

Note the reason I prefer a setter, is that it can be used from XML.

@dkaukov
Copy link
Contributor Author

dkaukov commented May 23, 2024

@gregw Could you please take a look now.

@gregw gregw self-requested a review May 23, 2024 07:13
gregw
gregw previously approved these changes May 23, 2024
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me!

@gregw
Copy link
Contributor

gregw commented May 23, 2024

CI failure is a flake. rerunning

@janbartel
Copy link
Contributor

Re-running CI test again.

@gregw gregw self-requested a review August 28, 2024 21:57
@gregw gregw self-assigned this Aug 28, 2024
@gregw
Copy link
Contributor

gregw commented Aug 29, 2024

Known CI flake

@gregw gregw merged commit 4755fa3 into jetty:jetty-12.0.x Aug 29, 2024
7 of 10 checks passed
@gregw
Copy link
Contributor

gregw commented Aug 29, 2024

@dkaukov thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants