actix-web icon indicating copy to clipboard operation
actix-web copied to clipboard

fix erroneous 500 responses when connection is unclean

Open robjtede opened this issue 2 years ago • 4 comments

PR Type

Fix

PR Checklist

  • [x] Tests for the changes have been added / updated.
  • [x] Documentation comments have been added / updated.
  • [x] A changelog entry has been made for the appropriate packages.
  • [x] Format code with the latest stable rustfmt.
  • [x] (Team) Label with affected crates and semver status.

Overview

The fix for #2624 tipped the scale too far the other way and caused 500 error responses to get queued. This moves the unclean connection detection to after the responses are queued and adds Connection: close headers.

Expect some minor perf drop (untested) but this is an important fix.

fixes #2695

robjtede avatar Apr 25 '22 13:04 robjtede

Regarding my previous comment, I don't think this is the case. We have deployed this fix (from this PR) in production at Modrinth, and users still are getting payload drops on their multipart requests.

This is a really big issue as when the payload drops and actix closes the connection, CORS headers aren't set so we can't access the response. This PR hasn't fixed the bug in my issue.

I feel like the previous approach was a lot better. The previous "fix" was for a very small use case. This issue is affecting big projects like MeiliSearch, and I don't think it fits with the actix architecture.

IMO, if I return an error in my response to a user, they should get that error. The connection shouldn't be instantly closed.

(Sorry if this wording was harsh- it's just a pretty big issue my team and I have been dealing with for the past couple of months)

Geometrically avatar Jun 09 '22 00:06 Geometrically

I understand the severity of the issue (we're hitting this at my work as well) and also that this PR is an incomplete fix, hence why it is not merged and released.

I'm going to revert the change that created this problem, which will give us back the other bug that existing in v3 (citation needed) which was relatively less problematic. This will give me and anyone else who wants to try some more time to debug and fix both problems finally.

Expect v4.1 to be out by EOD Thursday.

robjtede avatar Jun 09 '22 01:06 robjtede

Thanks a lot for your work on actix :smile:

Glad this issue is being fixed and love the work the team is doing. Besides this, actix has been an amazing platform for us to build our API on, and for that I am very thankful :heart:

Can't wait for 4.1!

Geometrically avatar Jun 09 '22 01:06 Geometrically

Hey, I noticed this was moved to v4.2. Is there a fix for this issue being planned? Or at least a fork we can use which resolves it to the prior system?

Geometrically avatar Jul 04 '22 17:07 Geometrically