Collect `env` processing into `client_env.rb` #3582

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

lib/puma/server.rb

@@ -576,6 +576,7 @@ def lowlevel_error(e, env, status=500)

    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

lib/puma/client.rb

+    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

lib/puma/client.rb

+      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...

…Parser

…single.rb

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

Open

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

Create client_env.rb - contains env code removed from request.rb

8ea6dc1

MSP-Greg added 28 commits September 4, 2025 12:41

request.rb - remove methods now contained in client_env.rb

5f3f99f

client.rb - adjustments for using client_env.rb

35bc5d2

client.rb - rework excessive content handling

a7af258

client.rb - remove @parsed_bytes >= MAX_HEADER checks, done by Http…

0ec7348

…Parser

client.rb - update class comments

98f0c28

request.rb - remove excessive content handling

d50d878

request.rb - move uncork_socket

3196b03

server.rb - rework excessive content handling

9d370e1

server.rb - rework response_to_error to match previous 413 behavior

001945b

request.rb - move HTTP method check to Client

4afb475

client.rb - move HTTP method check to Client

b54e6d2

server.rb - move HTTP method check to Client

54ea064

ci: test_puma_server.rb - rework 'excessive content' handling

c6bb3a8

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

ae23d09

…single.rb

ci: Delete test_normalize.rb

662e3dd

ci: test_request_invalid_multiple.rb - enable tests, etc

311f4ec

ci: test_out_of_band_server.rb - use puma_socket.rb

8badd3e

ci: test_puma_server_hijack.rb - use GET_1* constants

705dc7c

ci: runner - output java pid

c174b89

ci: test_puma_server_hijack.rb - adjustments for non MRI Rubies

e0f7276

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

d3963ac

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

4236690

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

ccbfe7f

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

21f70c2

client.rb - rename local variable body to parser_body

2214cb2

client.rb - use env_set_http_version from Server

09a3506

server.rb - send env_set_http_version to Client

c5d7b61

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

2c08d89

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

Collect env processing into client_env.rb #3582

Are you sure you want to change the base?

Collect env processing into client_env.rb #3582

Uh oh!

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!

Uh oh!

Collect `env` processing into `client_env.rb` #3582

Collect `env` processing into `client_env.rb` #3582

MSP-Greg commented Dec 22, 2024 •

edited

Loading

MSP-Greg commented Aug 12, 2025 •

edited

Loading