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.

Sign up for GitHub

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

Jump to bottom

Make HTTP length constants configurable for JRuby #3518

Merged
merged 3 commits into from
Nov 15, 2024
Merged

Make HTTP length constants configurable for JRuby #3518

merged 3 commits into from
Nov 15, 2024

Conversation

roque86
Copy link
Contributor

@roque86 roque86 commented Oct 13, 2024 •
edited
Loading

Description

Refactor the code to configure HTTP length constants through environment variables or system properties.
For this purpose, I added helper methods getEnvOrProperty and getConstLength, with default values and error handling for invalid inputs.

The changes consider the following three constants described in docs/compile_options.md:

  • PUMA_QUERY_STRING_MAX_LENGTH
  • PUMA_REQUEST_PATH_MAX_LENGTH
  • PUMA_REQUEST_URI_MAX_LENGTH

Documentation added as /docs/java_options.md

closes #3512

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@roque86 roque86 force-pushed the issue-3512_http_const_le branch 3 times, most recently from 95c3a17 to 5fcca57 Compare October 14, 2024 14:24
@roque86
Make HTTP length constants configurable
6d04c1c 
Refactor the code to make HTTP length constants configurable through environment variables or system properties.

Added helper methods `getEnvOrProperty` and `getConstLength` for this purpose, with default values and error handling for invalid inputs.
@roque86
Enhance Http11#getConstLength validation and add integration test
00e0d52 
Refactor `getConstLength` to ensure non-negative integer parsing and improve error handling. Added a new integration test to verify the proper behavior and logging of invalid environment variable values.
@roque86 roque86 force-pushed the issue-3512_http_const_le branch from 5fcca57 to 00e0d52 Compare October 14, 2024 14:40
@MSP-Greg
Copy link
Member

@roque86

Nice test. Glad to see you used 'integration', which is the preferred way to test ENV changes.

The one test that failed in 'NON-MRI: ubuntu-22.04 jruby' is something that I'll fix soon...

@roque86
Add documentation for Java options
b364b29 
Create a new `docs/java_options.md` to describe available system properties and environment variables for modifying Puma's Java extension.
@roque86
Copy link
Contributor Author

roque86 commented Oct 14, 2024

Thanks @MSP-Greg. What about Tests / MRI: macos-14 head (pull_request) ? Is that something that has already been looked at?

TestLauncher#test_puma_stats_clustered:
TimeoutEveryTestCase::TestTookTooLong: execution expired

Minitest::Skip: Kernel.fork isn't available on jruby on java

@roque86

Nice test. Glad to see you used 'integration', which is the preferred way to test ENV changes.

The one test that failed in 'NON-MRI: ubuntu-22.04 jruby' is something that I'll fix soon...

@MSP-Greg
Copy link
Member

Is that something that has already been looked at?

We run much of the test suite parallel, and if there's any blocking anywhere, tests may intermittently fail. That, plus a few more issues, is being worked on. Also, the CI runners often have 'noisy neighbors' affecting them.

Or, often one can run tests locally for days without failure, but use a GHA runner and things start intermittently failing...

@roque86 roque86 marked this pull request as ready for review October 14, 2024 18:05
@dentarg dentarg added the jruby label Oct 14, 2024
@dentarg dentarg changed the title Make HTTP length constants configurable Make HTTP length constants configurable for JRuby Oct 14, 2024
@dentarg dentarg added the waiting-for-review Waiting on review from anyone label Oct 14, 2024
@roque86
Copy link
Contributor Author

roque86 commented Nov 14, 2024

@MSP-Greg is there any pending action preventing us from moving this forward or do we only need to wait for the release cycles now?

@MSP-Greg MSP-Greg merged commit fba741b into puma:master Nov 15, 2024
67 of 68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MSP-Greg MSP-Greg MSP-Greg approved these changes

Assignees
No one assigned
Labels
jruby waiting-for-review Waiting on review from anyone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change QUERY_STRING max length for JRuby
3 participants
@roque86 @MSP-Greg @dentarg