Add log_client_closures http option #397
Conversation
| @@ -2150,7 +2183,7 @@ defmodule HTTP1RequestTest do | |||
|
|
|||
| assert output =~ "(Bandit.HTTPError) closed" | |||
| refute output =~ "IMPOSSIBLE" | |||
| assert ThousandIsland.connection_pids(context.server_pid) == {:ok, []} | |||
| assert ThousandIsland.connection_pids(context[:server_pid]) == {:ok, []} | |||
There was a problem hiding this comment.
I had to add this because I believe the http_server functionat the beginning of the test convertscontext` from a map into a keyword list. I was getting the following error, so I had to make this change:
** (KeyError) key :server_pid not found in: [base: "http://localhost:35985", port: 35985, server_pid: #PID<0.531.0>]
If you are using the dot syntax, such as map.field, make sure the left-hand side of the dot is a map
test/bandit/http1/request_test.exs
Outdated
| @@ -2159,6 +2192,7 @@ defmodule HTTP1RequestTest do | |||
| end | |||
|
|
|||
| test "raises an error if client closes while body is being written", context do | |||
| context = http_server(context, http_options: [log_client_closures: true]) | |||
There was a problem hiding this comment.
I'm turning on the option here because we assert that the error happened by capturing the logs, so we need to log the closure in these existing tests.
|
@mtrudel I ended up not being able to test the HTTP2 part, let me know if you can think of a way to test this, and feel free to add changes. |
|
Refactored the implementation to be shared (I've been meaning to do that for a while anyway!) and added some docs. Pending your approval of these changes we can get this merged! |
|
I can only see my single commit in this PR, not sure if you've pushed your changes? I approve whichever changes you feel are appropriate to get this merged, so feel free to push + merge :) Thank you! |
|
Huh, you're right - GitHub didn't allow me to push the changes up. Did you enable 'allow edits' per https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork? |
|
That option was preselected, it was enabled when I submitted the PR and it looks like it still is: |
|
I've done the disable / re-enable thing on the checkbox, plus I've invited you as a collaborator on my fork to see if that fixes it. I've done a couple of PRs after forking a repo and I've never had this issue before 🤔 |
It seems to happen about 25% of the time, not entirely sure why. In any case, i seem to have messed it up even worse 😠 |
|
Eesh. Fixed now, ready for review. |
|
Good call making this new option have the same values as The docs bit completely slipped my mind 😅 |
mtrudel
force-pushed
the
main
branch
2 times, most recently
from
12d7cc5 to
8dc344f
Compare
|
Thanks for the PR! I'm going to bundle this with a few other pending / recent changes and do a release this coming weekend. |
Created as a result from this discussion
This PR adds a new http option,
log_client_closures, that allows users to silenceBandit.HTTPerrors that stem from clients closing the connection. The option defaults tofalse.