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

Improve Jetty connection configuration by exposing a builder on Jetty's HttpConfiguration object #4387

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amccague
Copy link
Contributor

@amccague amccague commented Oct 9, 2024

Improve Jetty connection configuration by exposing a builder on Jetty's HttpConfiguration object

Would address the concern of https://youtrack.jetbrains.com/issue/KTOR-27/Increase-max-HTTP-header-size-for-server (which was closed as a duplicate for a Netty specific issue)

Subsystem
Jetty and Jetty Jarkarta Server

Motivation
Our particular use case was for configuring more header size on the Jetty Server from its default of 8KB.

Solution
Given the large number of options available on https://github.com/jetty/jetty.project/blob/ea258a362100ce21da02a960bee66dab745c4c99/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java .
Rather than exposing this as a specific configuration, it felt more flexible to provide a builder on the HttpConfiguration itself and apply that as part of Server Initialization.

Additionally compared to the existing configureServer approach it enables modifying the connectors configured by the ktor framework with minimal mutation/intervention by the application developer.

Comment on lines 57 to +58
initializeServer(configuration)
configuration.configureServer(this)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Porting this change here #4367 to Jetty Jarkarta (I did not notice this update in the first instance)

@amccague amccague force-pushed the configure_jetty_server_connection branch 2 times, most recently from f831b02 to 2dd1a4a Compare October 9, 2024 16:15
@amccague amccague force-pushed the configure_jetty_server_connection branch from 2dd1a4a to a9dbb73 Compare October 9, 2024 16:42
@e5l e5l self-requested a review October 17, 2024 05:29
Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

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

Hey @amccague, thank you for the PR. LGTM
Will be merged in 3.1.0-eap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants