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

fix: set error to Payload before dispatcher disconnect

Open ihciah opened this issue 2 years ago • 5 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

Now actix-web decode body with Content-Length like that:

  1. PayloadDeocder: Use Framed in an unconventional way which maintains data length left and returns Ok(Some(Bytes)) even when data is not sufficient. Returns PayloadItem::Chunk or PayloadItem::Eof.
  2. Codec: Convert PayloadItem to Message::Chunk(Some(Bytes)) or Message::Chunk(None).
  3. poll_request: Handling Message, for Message::Chunk(Some(..)), call feed_data; for Message::Chunk(None), call feed_eof.
  4. poll: call poll_request.

However, when connection closed, we have to tell Payload. So here( https://github.com/actix/actix-web/blob/ce3af777a0/actix-http/src/h1/dispatcher.rs#L1139 ) actix-web call feed_eof. But since the content length infomation is only inside the decoder, and the outside can only sense it by waiting the Stream ends. And here the stream ends in advance when connection closed.

If the payload sender exists, it means there's still data to receive, so we can just set error here before waking the receiver.

Close #3067

ihciah avatar Jul 10 '23 07:07 ihciah

I'm apprehensive to accept PRs like this without a test case that fails before the patch.

robjtede avatar Jul 19 '23 20:07 robjtede

Here is the case I think: https://github.com/actix/actix-web/issues/3067

This bug also caused an accident in our product environment.

ihciah avatar Jul 27 '23 07:07 ihciah

I added a test case to prove the fix. Before the patch it fails. Also the changelog is updated.

Note: This test might be a bit verbose as I'm not familiar with some of the actix built-in utils.

ihciah avatar Jul 27 '23 08:07 ihciah

This issue may cause server writing wrong data to db in the case of unstable network. Do you think we should publish a new version for it? Theoretically all users could be affected by this.

ihciah avatar Jul 27 '23 08:07 ihciah

ping @robjtede Can you review it?

ihciah avatar Aug 25 '23 09:08 ihciah