https://github.com/hyperium/hyper/pull/3185

[juntao]

No description provided.

[juntao]

flows review

[juntao]

Hello, I am a serverless review bot on flows.network. Here are my reviews of changed source code files in this PR.

---

src/proto/h1/conn.rs

This patch introduces changes related to the handling of header read timeouts in an HTTP/1 server:

1. Adds a new Instant import from std::time for managing the deadline of the header read timeout.
2. Modifies the Conn::read_head function to handle header read timeouts:
   • If the header read timeout is enabled and not running, the function calculates the deadline from the current time and the header read timeout duration.
   • If the header read timeout feature is set and the future is unset, it creates and stores a new timer for the header read timeout using the sleep_until function.
   • If the timer exists, the function will reset the timer to the new deadline.
   • Changes the handling of the result from self.io.parse::<T>: Instead of using ready!, it directly matches the Poll enum.
     • In case of Poll::Pending, it checks if the header read timeout is running and if so, it polls the future. If the future is ready, the header read timeout is stopped, a warning is logged, and an error is returned.
3. Resets the header read timeout running flag and clears the header read timeout future after a successful header read.

In summary, the patch adds more refined handling of header read timeouts in servers based on the HTTP/1 protocol. It sets, resets, and manages timeouts and related timers more accurately to handle slow-or-stuck clients during header reads.

src/proto/h1/io.rs

The source code snippet provided is a part of a buffered reader and writer for a Tokio-based application. The code appears well-organized and documented. However, there are a few minor improvements that can be made:

1. The use of #[cfg(feature = "client")] is seen a few times in the code. Using features as a conditional compilation directive makes it difficult to read and maintain the code. Instead, it is better to separate the client and server code into distinct modules or components.

2. fn poll_flush has two different implementations based on the WriteStrategy. Flushing code could be separated into two separate internal methods for each write strategy (e.g., fn poll_flush_flatten and fn poll_flush_queue). These methods could then be called from fn poll_flush based on the WriteStrategy.

3. Import usage formatting: Prefer grouping imports from the same parent module on the same line, separated by comma for better readability.
   For example:

   use std::{cmp, fmt, mem::MaybeUninit};

   Instead of having separate lines for each import.

4. The method fn incr_power_of_two and fn prev_power_of_two names are not very descriptive. Consider renaming to more descriptive names like fn double_size and fn halve_size.

5. Use u64 instead of usize for calculating buffer capacity related logic. This helps in avoiding any potential issues related to bitness (32-bit vs 64-bit systems).

The key changes in the provided patch are:

1. Removed the import of std::future::Future as it's no longer used in the code.
2. Removed the repeated use of #[cfg(feature = "server")] in the parser context structure and different configurations related to it.
3. Simplified the conditions related to header read timeout and removed associated logic in poll_advance method, implying the header read timeout is no longer handled in this part of the code.
4. Removed crate::common::time::Time import in the tests module as it's no longer used.
5. Removed the fields related to h1_header_read_timeout and its associated future, running configuration, and timer fields in the tests module.

src/proto/h1/mod.rs

The key changes in the provided patch are:

1. Removed conditional imports #[cfg(feature = "server")] for std::pin::Pin, std::time::Duration, crate::common::time::Time, and crate::rt::Sleep. This implies that these imports and related configurations are no longer conditionally included based on the "server" feature.
2. Removed the fields related to h1_header_read_timeout, h1_header_read_timeout_fut, h1_header_read_timeout_running, and timer from the ParseContext struct. This indicates that header read timeout logic and configurations are no longer part of the ParseContext struct.

src/proto/h1/role.rs

1. The encode_headers_with_lower_case function is missing the closing curly brace }. This will result in a syntax error in the program.

2. Using MaybeUninit can be dangerous as it can leave the memory uninitialized, and any access to uninitialized memory can lead to undefined behavior. In this code, there are notes about the usage of MaybeUninit indicating that the uninitialized memory is never accessed until after the httparse library assigns the values. The comments do provide some level of assurance, but any change in the associated code may introduce risks. It is recommended to use safer constructs whenever possible and be extra careful when dealing with code involving MaybeUninit.

3. Using unsafe code like in the header_value! macro may lead to possible memory safety bugs if not handled correctly. In this case, once the values have been checked and parsed, the HeaderValue::from_maybe_shared_unchecked function is called unsafely. While it seems to be correct in this context, extra caution should be taken when handling unsafe code in the future.

4. The macro maybe_panic has the potential to panic during runtime if it's running a debug build. Panicking might cause the application to terminate unexpectedly. Instead, consider returning an error result so the issue can be properly handled by the calling code.

5. The code contains several hardcoded values for maximum headers count and maximum URI length, which might not be ideal for all use cases. It is better to make those values configurable as constants or configuration parameters.

6. The naming convention of some variables is not consistent with common Rust guidelines (e.g., is_http_11 vs is_http11 or keep_alive vs. is_keep_alive). Consistent naming conventions improve code readability and maintainability.

Overall, the code snippet seems to be implementing an HTTP/1.1 parser for headers. The design and logic appear to be fine. However, there are some potential problems, particularly regarding memory safety and error handling, which should be addressed to improve the security and robustness of the code.

This patch introduces key changes in the code, which include:

1. The removal of the use std::time::Instant; import statement since it's no longer needed due to the removal of the related code.
2. The removal of time-related code, specifically related to the HTTP/1.1 header read timeout functionality. This includes removing several lines of code that set, reset, and configure timers for header read timeouts.
3. The removal of references to the Time::Empty timer from the testing module as they are no longer needed following the removal of the header read timeout functionality.

These changes primarily focus on removing the HTTP/1.1 header read timeout functionality and associated code, simplifying the codebase by reducing timer complexity.

tests/server.rs

The code appears to be a Rust implementation of an HTTP server with several unit tests provided. Here are some potential issues and improvements that can be made:

1. In the get_should_ignore_body test, the body of the GET request is being ignored, but a better approach would be to return an error if there is an unexpected body for the GET request.

2. In the get_with_body test, the body in the GET request is being read with a specified content length, but it might be beneficial to check if the content length matches the actual length of the body to avoid processing incorrect data.

3. There is a lot of repetition in the response_body_lengths submodule tests. Creating a helper function to reduce code duplication would improve readability and maintainability.

4. In the post_with_chunked_body test, a possible improvement would be verifying that the server returns an appropriate response, in addition to checking the server body.

5. Most tests don't have error-handling or assertions checks while reading or writing sockets. Encapsulating socket operations in helper functions with error handling can help avoid potential test failures due to unhandled errors.

6. Some of the error messages in assertion checks do not provide enough context (e.g., "error should be overflow: {:?}"). Improving the error messages with more context will help in diagnosing test failures.

7. The run_test function in the response_body_lengths submodule makes it difficult to understand the actual logic of each test case due to the varying input parameters. Refactoring test cases into individually smaller functions can help with readability and maintaining code.

8. Instead of using unwrap() calls throughout the tests, using the ? operator or converting the tests to async tests and using .await can help handle errors more gracefully.

9. The tests use a mix of tokio and std::thread calls. It would be better to choose a consistent method of working with threads and asynchronous programming within the tests.

The patch adds a new test named header_read_timeout_starts_immediately to the test suite. This test checks whether the read timeout starts immediately upon connection. It sets up a TCP listener and starts a new thread that connects to the listener, sleeps for 3 seconds, and then tries to read from the connection. Meanwhile, the main test function creates an HTTP server with a 2-second header read timeout and expects a timeout error. The test ensures that the header read timeout starts as soon as the connection is established, even before any data has been received.

cc hyperium/hyper#3185