search

mtrudel / bandit

Bandit is a pure Elixir HTTP server for Plug & WebSock applications
MIT License
1.67k stars 80 forks source link

Add log_client_closures http option #397

Closed goncalotomas closed 2 weeks ago

goncalotomas commented 3 weeks ago

Created as a result from this discussion This PR adds a new http option, log_client_closures, that allows users to silence Bandit.HTTP errors that stem from clients closing the connection. The option defaults to false.

goncalotomas commented 3 weeks ago

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

mtrudel commented 2 weeks ago

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!

goncalotomas commented 2 weeks ago

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!

mtrudel commented 2 weeks ago

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?

goncalotomas commented 2 weeks ago

That option was preselected, it was enabled when I submitted the PR and it looks like it still is:

image

I'll try disabling and re-enabling the option, but I don't know why this happened. 😕

goncalotomas commented 2 weeks ago

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 🤔

mtrudel commented 2 weeks ago

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 😠

mtrudel commented 2 weeks ago

Eesh. Fixed now, ready for review.

goncalotomas commented 2 weeks ago

Good call making this new option have the same values as log_protocol_errors! I struggled to decide whether or not to make it a simple boolean option but I agree this is more consistent with the existing behaviour.

The docs bit completely slipped my mind 😅 Looks good to me, feel free to merge!

mtrudel commented 2 weeks ago

Thanks for the PR! I'm going to bundle this with a few other pending / recent changes and do a release this coming weekend.