Add customizable error messages on thread shutdown

[zanker]

Description

When threads are forced to shutdown, the response is static and cannot be configured. For our use case, we want to always guarantee a JSON response is returned on the rare case this comes up.

I didn't make this a proc like lowlevel_error_handler, since the intent is to be shutting down quickly, a static response felt more appropriate. While I was here, I also added a test for the base case (default message) as I couldn't find any tests which exercised that.

Your checklist for this pull request

• I have reviewed the guidelines for contributing to this repository.
• I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
• I have added 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.

[nateberkopec]

Provided some code feedback but I'm not 100% sure on the usecase.

"we want to always guarantee a JSON response" -> Why? Isn't the 503 status code enough here?

[nateberkopec]

Actually, it doesn't have to be an Enumerable. It just has to respond to each.

[nateberkopec]

Let's set this as the default value of the @options, not hard-coded here.

[nateberkopec]

Then, if we decide that yes, it's important that Puma returns a particular content-type on low-level failures, I think instead we should change all low-level errors. There's a lot of them and this only configures one, which isn't that helpful.

Something like:

lowlevel_error_content_type = :json

Maybe we can support :json or :none for now.

Existing sites where we render lowlevel errors would be changed to look something like:

if @leak_stack_on_error
    generate_error_response(status: 500, body: "Puma caught this error: #{e.message} (#{e.class})\n#{e.backtrace.join("\n")}")
else
  generate_error_reponse(status: 500, body: "An unhandled lowlevel error occurred. The application logs may have details").
end

[dentarg]

Provided some code feedback but I'm not 100% sure on the usecase.

"we want to always guarantee a JSON response" -> Why? Isn't the 503 status code enough here?

I can understand the use-case, if you are building an API, and you document that your API return responses in JSON, it is unexpected when the response isn't in JSON. Maybe you want to include more information than the HTTP status code, e.g. your application can return HTTP status 503 for number of different errors, and you want to provide a certain error code in the error response. Then it makes sense to return the body in JSON, including that information.

[zanker]

@dentarg has it right. Returning static JSON of Puma's choosing would be an improvement over static plaintext, but it's not compatible with a JSON API if the API expects {error: true, msg: "foo bar"} and Puma returns {message: "foo bar"}.

I think it's ok if lowlevel_error has a static plaintext response, since you can always override it to be JSON, but I don't have strong feelings.

I made the changes requested, let me know if you have anymore feedback!

[nateberkopec]

Cool, ppl on Twitter agreed this is useful so we'll put it in. I need to pull it down and play w/implementation to see if there's a better way to integrate it w/the rest of our error handling.

[zanker]

Sounds good! Thanks for the fast response

[zanker-stripe]

Just to double check, I assume you don't need anything from me at this point since it's approved. You're just waiting on having time to play around with alternatives?

[nateberkopec]

@zanker-stripe yup, thanks!

[nateberkopec]

OK, so I took another look and here's what I think needs to happen:

1. Thread shutdown is getting a special-case path and avoiding lowlevel_error_handler. It should not do that and use lowlevel_error_handler instead.
2. To do that, we need lowlevel_error_handler to get a new argument, status, which should default to 500.
3. To deal with the new argument, lowlevel_error_handler needs a case to deal with an arity == 3 handler.

def lowlevel_error(e, env, status = 500)
      if handler = @options[:lowlevel_error_handler]
        if handler.arity == 1
          return handler.call(e)
        elsif handler.arity == 2
          return handler.call(e, env)
        elsif handler.arity == 3
          return handler.call(e, env, status)
        end
      end
    end

and the default config should have the following:

  DefaultLowlevelErrorHandler = lambda do |_e, _env, status|
      [status, {}, ["An unhandled lowlevel error occurred. The application logs may have details.\n"]].freeze
    end

[nateberkopec]

This way, if you want JSON responses, you can do that consistently for all error types.

[zanker]

@nateberkopec Do you have any concerns with the fact that this means people can accidentally run slower code during exit and essentially delay the shutdown timer?

I'm fine with it if you are, and I'll do a PR for that.

[nateberkopec]

I'm OK with that. We can document that this should return ASAP or the shutdown timer may be affected.

[nateberkopec]

People do talk to external APIs in their lowlevel_handlers often (Sentry, Rollbar, etc) but I don't think that possibly delaying the shutdown timer by up to a second is all that critical of an issue 🤷‍♂

[zanker-stripe]

Fair enough! #2203 will handle it that way.