fix erroneous 500 responses when connection is unclean #2745
Conversation
| connection: close | ||
| date: Thu, 01 Jan 1970 12:34:56 UTC | ||
|
|
||
| payload dropped |
There was a problem hiding this comment.
Does this code retain responses a server gives when a payload is dropped? That is the intended behavior IMO, and is what actix did before 4.0.
|
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) |
|
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. |
|
Thanks a lot for your work on actix 😄 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 ❤️ Can't wait for 4.1! |
|
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? |
add changelog add send error response state remove comment
robjtede
force-pushed
the
fix-dispatch-close
branch
from
4722d7f
to
dd8692e
Compare
PR Type
Fix
PR Checklist
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: closeheaders.Expect some minor perf drop (untested) but this is an important fix.
fixes #2695