Collect `env` processing into `client_env.rb` by MSP-Greg · Pull Request #3582 · puma/puma

MSP-Greg · 2024-12-22T23:20:43Z

Description

Currently, code that processes the request env is contained in both client.rb and request.rb. Move the code to client_env.rb, and add an additional test file test_request.rb. This doesn't require creating a server.

Will move more tests to the file, some from test_puma_server.rb and some from test_request_invalid.rb.

Closes #3540

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.

dentarg · 2024-12-25T15:19:50Z


    def response_to_error(client, requests, err, status_code)
      status, headers, res_body = lowlevel_error(err, client.env, status_code)
+      res_body = ["Payload Too Large"] if status == 413


Just double checking my understanding: So with this refactoring, the lowlevel_error is called (and potentially a user specified lowlevel_error_handler), which didn't happen before? In order to preserve the existing behaviour – the hard-coded response body Payload Too Large we set the body here

Question: Should we even call lowlevel_error at all when status_code is 413? If we didn't run the user specified lowlevel_error_handler before this refactoring, we probably shouldn't start doing it now.

A future (major?) Puma version could let the user specified lowlevel_error_handler to control the full response for the "Payload Too Large" scenario.

What if we introduce HttpParserError413 and raise that instead of HttpParserError (we already have HttpParserError501)? Then we can, in client_error, call prepare_response directly.

Or is the HttpParserError raise at https://github.com/puma/puma/pull/3582/files#r1897406430 problematic, in that either "Payload Too Large" or some other error can happen?

Is @error_status_code important or can we get by without it? I mean, can we pass the status code when we raise HttpParserError? I think that is a clearer interface.

Re adding HttpParserError413, I'm not sure about the idea of having error classes for every possible request error. That's why I added @error_status_code. We might need to add something like @error_status_message or something like it.

I didn't want this PR to touch error handling, but once I got into it, it couldn't be avoided.

How about changing the code to:

def response_to_error(client, requests, err, status_code) # @todo remove sometime later if status_code == 413 status = 413 res_body = ["Payload Too Large"] headers = {} else status, headers, res_body = lowlevel_error(err, client.env, status_code) end prepare_response(status, headers, res_body, requests, client) end

Yeah that will do it

But we could also pass information via the error object? Instead of adding @error_status_message, HttpParserError could hold status code and the message.

But we could also pass information via the error object?

I didn't state one goal, of which this is the start. I think 'ruby' error handling can be totally contained in the new method Client#process_env_body. I'd like to contain it all within Client, except it will need the call to generate the response.

dentarg · 2024-12-25T15:32:39Z

Re: 1f7a775 and 4f6b798, if done in one commit, git (on the command line, not github.com) can detect moved lines. That's really useful IMHO.

dentarg · 2024-12-25T15:34:53Z

+    def process_env_body
+      if above_http_content_limit(@parser.body.bytesize)
+        @http_content_length_limit_exceeded = true
+        @error_status_code = 413
+      end
+      temp = setup_body
+      normalize_env
+      req_env_post_parse
+      if @error_status_code
+        # @env[HTTP_CONNECTION] = 'close'
+        raise HttpParserError
+      end
+      temp
+    end


Can @error_status_code be anything other than 413 here?

Not with this PR. Future PR's that consolidate the request error handling will probably add more values.

Yeah that will do it

I added it and rebased.

dentarg · 2024-12-25T18:29:03Z

+      normalize_env
+      req_env_post_parse
+      if @error_status_code
+        # @env[HTTP_CONNECTION] = 'close'


Remove this, or is it a future TODO so you want to keep the reminder for it?

MSP-Greg · 2025-02-16T00:30:12Z

Questions about exception/error handling during request processing and up to and including calling the app.

I believe there are four types of exceptions:

A. A socket error.

B. An invalid request is submitted which does not meet the specs for a proper request.

C. A request is submitted which fails due to user defined constraints like http_content_length_limit or supported_http_methods.

D. The app raises an exception.

Questions:

1. When is/should low_level_errorcalled? The comment seems to state that it is only called when the app raises an exception. So, that would mean that only ‘D’ from above would trigger a low_level_error call? At present, I think some type ‘C’ errors are also calling low_level_error.

2. Previous discussions have been concerned with ‘finger printing’ that Puma is the web server. If so, should all ‘production’ error responses not include a response body/content?

3. Should users have control over which error types are logged? Or, should all be logged?

4. Similar to above, should users have control over which error types generate a response via the lowlevel_error_handler?

nateberkopec · 2025-02-18T05:40:57Z

woof, I always thought it was cases A/B/C 😆 I sort of always thought of it as puma_error_handler but maybe I'm wrong...
I swear that was already an issue in the past... can't find it now. Yes, probably?
I think we should allow the low_level_error handler to return false, people can filter stuff out in there if they want.
What's the current contract for the return value of low_level_error?

MSP-Greg · 2025-08-12T22:05:00Z

I'm working on a significant update to this, currently cleaning up tests.

When I'm finished, I'll help review PR's.

MSP-Greg · 2025-08-19T14:21:42Z

Today's Puma has grown quite a bit from its origins. Over time, server.rb (and also test_puma_server.rb) have often been the 'catch all' files for new features.

Back in October of 2020 (PR #2419), request.rb was added, which extracted most of the processing of the request and the generation of the response from server.rb. The file was included in Server, but it made it easier to keep track of request/response processing, leaving the code in server.rb to handle starts/restarts/stops, event loop, interact with the ThreadPool, etc.

Client contains the HttpParser, which reads and processes the request line and headers into the env info passed to the app. If the request contains content/body, Client reads it, and decodes it if needed.

Much of the code to modify and validate the env passed to the app was scattered between Client and Request. The code also raised errors when validation failed.

Since the HttpParser is contained in Client, this PR moves all the request related code to Client. Much of it is contained in the client_env.rb file, which is included in Client.

By encapsulating all the request code in Client, it becomes easier to consider changes. Also, since we can pass an IO like object (with a single request) to Client.new, the testing for single requests doesn't require creating a Server instance. See test/test_request_single.rb for examples. This removed about 100 lines of code from test/test_puma_server.rb.

There many tests that create an app to return a response of request env properties, send a single request, and then check the response. These can moved to test/test_request_single.rb. This PR moves some of them, but more remain.

Although not contained in this PR, renaming request.rb to response.rb would clear up its purpose. Best left for a time immediately before a new release.

MSP-Greg · 2025-08-19T14:49:51Z

Not sure whether to open an issue regarding this.

The issue is when should lowlevel_error_handler be called. I believe there are five cases:

The request is invalid based on RFC's.
The request exceeds hard-coded length/qty limits.
The request exceeds user defined length/qty limits (example - request content/body max size).
The app raises an error.
The return of the app is invalid.

Note that requests that are invalid based on 2 & 3 may be 'valid' requests, the limits may be set because they could be malicious requests.

This PR isn't tagged v7, but the refactoring is extensive...

…single.rb

MSP-Greg · 2026-02-05T16:41:26Z

@nateberkopec

Thanks. I merged 73b9bd0f. All of the code from that is fine.

The following code in client.rb isn't needed. It improperly handles pipelined requests. I removed it, and all tests still pass.

if above_http_content_limit(@parser.body.bytesize)
  @http_content_length_limit_exceeded = true
  @error_status_code = 413
end

MSP-Greg · 2026-02-15T20:54:54Z

See #3887

Stale

MSP-Greg added the refactor label Dec 23, 2024

github-actions Bot added the waiting-for-review Waiting on review from anyone label Dec 23, 2024

MSP-Greg force-pushed the 00-client-reorg branch from 245660e to 415eb00 Compare December 23, 2024 19:10

dentarg reviewed Dec 25, 2024

View reviewed changes

MSP-Greg force-pushed the 00-client-reorg branch from 415eb00 to 59b54fb Compare December 25, 2024 16:55

dentarg reviewed Dec 25, 2024

View reviewed changes

This was referenced Dec 30, 2024

Change HttpParserError to be subclass of StandardError #3590

Merged

Invalid request is swallowed and times out #3552

Closed

MSP-Greg mentioned this pull request Feb 1, 2025

Refactor HttpParserError handling, add test/test_request_single.rb #3586

Merged

7 tasks

MSP-Greg force-pushed the 00-client-reorg branch 4 times, most recently from d018dc6 to b1827cb Compare August 17, 2025 21:31

This was referenced Aug 19, 2025

Drop Ruby < 3.0 ? #3588

Closed

Update minimum Ruby version to 3.0 #3698

Merged

ci: misc test fixes and refactors #3694

Merged

MSP-Greg force-pushed the 00-client-reorg branch 3 times, most recently from ee2ab27 to 790e126 Compare August 22, 2025 18:29

MSP-Greg mentioned this pull request Aug 22, 2025

client.rb - rename local variable body to parser_body #3633

Closed

7 tasks

MSP-Greg force-pushed the 00-client-reorg branch from 790e126 to ea1d8e8 Compare August 26, 2025 01:42

MSP-Greg mentioned this pull request Aug 28, 2025

Don't set env['HTTP_VERSION'] for Rack >= 3.1 #3711

Merged

7 tasks

MSP-Greg force-pushed the 00-client-reorg branch from ea1d8e8 to 2c08d89 Compare September 4, 2025 18:50

MSP-Greg added 18 commits February 4, 2026 16:32

ci: move some tests from test/test_normalize.rb to test/test_request_…

8b3898b

…single.rb

ci: Delete test_normalize.rb

d67b883

ci: test_request_invalid_multiple.rb - enable tests, etc

efe795b

ci: test_out_of_band_server.rb - use puma_socket.rb

85cbe99

ci: test_puma_server_hijack.rb - use GET_1* constants

04dfcd9

ci: test_puma_server_hijack.rb - adjustments for non MRI Rubies

57e3082

ci: test_request_single.rb - move HTTP method check to Client

9192747

ci: test_request_single.rb - move request tests from test_puma_server.rb

9f5ac2d

ci: test_puma_server.rb - remove tests moved to test_request_single.rb

f9d8c83

ci: test_request_single.rb - use 'Socket.pair'

10f05dc

client.rb - rename local variable body to parser_body

f440cbe

client.rb - use env_set_http_version from Server

8545261

server.rb - send env_set_http_version to Client

3b426ad

client_env.rb - set @env[HTTP_VERSION] if @env_set_http_version

da37b6e

client.env - revise default_server_port per PR puma#3732

8171e81

Rename request.rb to response.rb

4aa5710

client.rb - revise content length limit exceeded

e9ee22b

ci: test_request_single.rb - add TestRequestReset

bc0fce8

MSP-Greg force-pushed the 00-client-reorg branch from a4e7e02 to bc0fce8 Compare February 4, 2026 22:40

CI: test_request_single.rb - minor cleanup

1152422

github-actions Bot added waiting-for-review Waiting on review from anyone and removed waiting-for-changes Waiting on changes from the requestor labels Feb 12, 2026

nateberkopec merged commit 73abd70 into puma:main Feb 22, 2026
85 checks passed

MSP-Greg mentioned this pull request Feb 22, 2026

How to set a limit for http body #3834

Open

dentarg removed the waiting-for-review Waiting on review from anyone label Feb 23, 2026

MSP-Greg mentioned this pull request Feb 23, 2026

client.rb - dry up above_http_content_limit #3889

Merged

7 tasks

hayat01sh1da mentioned this pull request Apr 10, 2026

[ruby-on-rails] Bump Puma to 8.0.0 hayat01sh1da/botpress-accuracy-checkers#90

Merged

Conversation

MSP-Greg commented Dec 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Your checklist for this pull request

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dentarg commented Dec 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSP-Greg commented Feb 16, 2025

Uh oh!

nateberkopec commented Feb 18, 2025

Uh oh!

MSP-Greg commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MSP-Greg commented Aug 19, 2025

Uh oh!

MSP-Greg commented Aug 19, 2025

Uh oh!

MSP-Greg commented Feb 5, 2026

Uh oh!

MSP-Greg commented Feb 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MSP-Greg commented Dec 22, 2024 •

edited

Loading

MSP-Greg commented Aug 12, 2025 •

edited

Loading