Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

revert broken fix in #2624 #2779

Merged
merged 3 commits into from Jun 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions actix-http/CHANGES.md
Expand Up @@ -4,6 +4,11 @@
### Changed
- Minimum supported Rust version (MSRV) is now 1.56 due to transitive `hashbrown` dependency.

### Fixed
- Revert broken fix in [#2624] that caused erroneous 500 error responses. Temporarily re-introduces [#2357] bug. [#2779]

[#2779]: https://github.com/actix/actix-web/issues/2779


## 3.0.4 - 2022-03-09
### Fixed
Expand Down
73 changes: 4 additions & 69 deletions actix-http/src/h1/dispatcher.rs
Expand Up @@ -15,14 +15,14 @@ use bitflags::bitflags;
use bytes::{Buf, BytesMut};
use futures_core::ready;
use pin_project_lite::pin_project;
use tracing::{debug, error, trace};
use tracing::{error, trace};

use crate::{
body::{BodySize, BoxBody, MessageBody},
config::ServiceConfig,
error::{DispatchError, ParseError, PayloadError},
service::HttpFlow,
ConnectionType, Error, Extensions, OnConnectData, Request, Response, StatusCode,
Error, Extensions, OnConnectData, Request, Response, StatusCode,
};

use super::{
Expand Down Expand Up @@ -691,74 +691,12 @@ where
let can_not_read = !self.can_read(cx);

// limit amount of non-processed requests
if pipeline_queue_full {
if pipeline_queue_full || can_not_read {
return Ok(false);
}

let mut this = self.as_mut().project();

if can_not_read {
debug!("cannot read request payload");

if let Some(sender) = &this.payload {
// ...maybe handler does not want to read any more payload...
if let PayloadStatus::Dropped = sender.need_read(cx) {
debug!("handler dropped payload early; attempt to clean connection");
// ...in which case poll request payload a few times
loop {
match this.codec.decode(this.read_buf)? {
Some(msg) => {
match msg {
// payload decoded did not yield EOF yet
Message::Chunk(Some(_)) => {
// if non-clean connection, next loop iter will detect empty
// read buffer and close connection
}

// connection is in clean state for next request
Message::Chunk(None) => {
debug!("connection successfully cleaned");

// reset dispatcher state
let _ = this.payload.take();
this.state.set(State::None);

// break out of payload decode loop
break;
}

// Either whole payload is read and loop is broken or more data
// was expected in which case connection is closed. In both
// situations dispatcher cannot get here.
Message::Item(_) => {
unreachable!("dispatcher is in payload receive state")
}
}
}

// not enough info to decide if connection is going to be clean or not
None => {
error!(
"handler did not read whole payload and dispatcher could not \
drain read buf; return 500 and close connection"
);

this.flags.insert(Flags::SHUTDOWN);
let mut res = Response::internal_server_error().drop_body();
res.head_mut().set_connection_type(ConnectionType::Close);
this.messages.push_back(DispatcherMessage::Error(res));
*this.error = Some(DispatchError::HandlerDroppedPayload);
return Ok(true);
}
}
}
}
} else {
// can_not_read and no request payload
return Ok(false);
}
}

let mut updated = false;

// decode from read buf as many full requests as possible
Expand Down Expand Up @@ -904,10 +842,7 @@ where
if timer.as_mut().poll(cx).is_ready() {
// timeout on first request (slow request) return 408

trace!(
"timed out on slow request; \
replying with 408 and closing connection"
);
trace!("timed out on slow request; replying with 408 and closing connection");

let _ = self.as_mut().send_error_response(
Response::with_body(StatusCode::REQUEST_TIMEOUT, ()),
Expand Down
3 changes: 3 additions & 0 deletions actix-http/src/h1/dispatcher_tests.rs
Expand Up @@ -783,6 +783,9 @@ async fn upgrade_handling() {
.await;
}

// fix in #2624 reverted temporarily
// complete fix tracked in #2745
#[ignore]
#[actix_rt::test]
async fn handler_drop_payload() {
let _ = env_logger::try_init();
Expand Down