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

Bump hyper version to 1.0 #20

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

L-jasmine
Copy link
Contributor

No description provided.

Copy link
Member

juntao commented Jan 22, 2024

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


.cargo/config.toml

The provided source code appears to be a configuration file for a Rust project. Specifically, it seems to configure the build process for a target of "wasm32-wasi" and sets the "runner" to use "wasmedge".

At first glance, there are no immediate issues with the code snippet. However, without additional context or the complete codebase, it is difficult to determine if there are any potential problems.

Here are a few points to consider when reviewing such configuration files:

  1. Verify the correctness of the target platform: Ensure that the selected target platform ("wasm32-wasi" in this case) is the intended one for the project. Confirm that all the required dependencies and libraries are available for this platform.
  2. Validate the usage of "rustflags": The "rustflags" option is used to specify custom flags for the Rust compiler. Check if the "--cfg tokio_unstable" flag is required and appropriate for the project. Consider whether it's necessary to use an unstable feature like Tokio in this case.
  3. Confirm the correctness of the "runner" setting: The "runner" option tells the build system or toolchain which program should be used to execute the compiled WASM binary. Ensure that "wasmedge" is the correct runner for the given environment and purpose.

It is important to view this code snippet within the context of the entire project to ensure its correctness and appropriateness.

The key changes in the provided patch are as follows:

  1. The indentation of the "target" attribute under the "[build]" section has been fixed.
  2. A new "rustflags" attribute has been added under the "[build]" section with the value "--cfg tokio_unstable".
  3. The "runner" attribute under the "[target.wasm32-wasi]" section has been modified to specify "wasmedge" as the runner program.

These changes are related to the "Bump hyper version to 1.0" task as they update the target platform and add a Rust flag for using the unstable feature "tokio_unstable". The modification to the "runner" attribute is not directly related to the task.

client-https/Cargo.toml

The code snippet you provided appears to be a Cargo.toml file, which is used to declare dependencies for a Rust project. The purpose of this Cargo.toml file seems to be defining the dependencies and their versions for a crate named "wasmedge_hyper_client_https".

Here are a few potential problems or improvements I noticed:

  1. Dependency versions:

    • The hyper dependency has a version specified as "1" with features set to ["full"]. It is recommended to specify the complete version number instead of just "1" to avoid any conflicts in future updates. For example, you can use hyper = "0.14.10" if you want to use version 0.14.10.

    • The tokio dependency has a version specified as "1" with features set to ["rt", "macros", "net", "time", "io-util"]. Again, it is recommended to specify the complete version number for better accuracy and stability.

  2. Unused dependencies:

    • The pin-project and http-body-util dependencies are listed but not used in the code snippet. It is advisable to remove any unused dependencies to keep the codebase cleaner.
  3. Miscellaneous:

    • The pretty_env_logger dependency is specified as "0.4.0". You might want to ensure that this version is compatible with the other dependencies in your project.

    • It's unclear whether the wasmedge_wasi_socket package is necessary for your project. If it's not used, it can be removed to reduce unnecessary dependencies.

These are the notable points based on the provided code snippet. However, without the complete code and context, it's challenging to provide a thorough review. It's always recommended to test the code against the specified dependencies to ensure compatibility and functionality.

The key changes in the patch are as follows:

  1. Dependency versions:

    • The hyper_wasi dependency is replaced with hyper version 1.

    • The http-body-util dependency is downgraded from version "0.1.0-rc.2" to "0.1.0".

    • The tokio_wasi dependency is replaced with tokio version 1.

  2. Additional dependencies:

    • The wasmedge_wasi_socket, pin-project, tokio-rustls, webpki-roots, and rustls dependencies are added.
  3. Removed dependencies:

    • The wasmedge_rustls_api and wasmedge_hyper_rustls dependencies are removed from the original Cargo.toml file.

These are the key changes made in the patch. It's important to note that this information is based on the provided patch, and without the complete code and context, it's challenging to provide a comprehensive review.

client-https/src/main.rs

Overall, the code looks fine and handles the task of fetching an HTTPS URL correctly. Here are a few potential problems and suggestions for improvement:

  1. Unused import: The AsyncWriteExt trait from the tokio::io module is imported but not used. It can be removed to avoid clutter and improve code readability.
  2. Error handling improvement: In the fetch_https_url function, there are a few places where errors are unwrapped (unwrap) directly. It is recommended to use proper error handling with ? operator or use additional match statements to handle errors properly. This will help in propagating errors and providing better error messages.
  3. Logging: The pretty_env_logger::init() call is present, but there are no log messages being generated. Consider adding appropriate log statements to provide better visibility into the code execution and potential errors.
  4. Exception handling: The code uses the expect method to handle situations where the optional properties of the provided URI return None. Consider using proper error handling techniques instead of panicking.

Apart from these suggestions, the code looks good and should work as intended for fetching and printing responses from HTTPS URLs.

The key changes in the patch are as follows:

  1. Added #![deny(warnings)] and #![warn(rust_2018_idioms)] to enforce stricter warnings and idiomatic Rust code practices.
  2. Imported additional modules and types required for the code changes.
  3. Created a new TokioIo struct to implement the hyper::rt::Read and hyper::rt::Write traits for the tokio::io::AsyncRead and tokio::io::AsyncWrite traits, respectively.
  4. Changed the return type of the main function to MainResult<()>, which is a custom result type alias.
  5. Added logging initialization with pretty_env_logger::init().
  6. Modified the fetch_https_url function to use the new TokioIo struct for reading and writing data from the TLS stream.
  7. Created a TLS connector using rustls::ClientConfig and tokio_rustls::TlsConnector to establish a secure connection with the remote server.
  8. Created a raw TCP stream using wasmedge_wasi_socket::TcpStream and converted it to a TcpStream from the tokio::net module.
  9. Implemented the HTTP/1.1 handshake using hyper::client::conn::http1::handshake.
  10. Replaced the usage of hyper::Client with manual request and response handling using sender.send_request() and consuming the response frame by frame.
  11. Replaced the previous method of reading the response body using hyper::body::to_bytes and instead stored the response data in a Vec<u8> for printing later.

Overall, the patch modifies the code to handle HTTPS connections using the tokio and hyper crates with the help of the rustls TLS library. It also introduces better error handling and logging.

client/Cargo.toml

The code provided appears to be a configuration file named Cargo.toml for a Rust project. It specifies the dependencies for the project.

Here are a few potential issues:

  1. The hyper dependency is using a non-specific version "1". It would be better to specify a specific version to ensure compatibility and avoid unexpected breaking changes. For example, specifying version = "1.0.0".

  2. The tokio dependency is using a non-specific version "1". Similar to hyper, it would be better to specify a specific version to ensure compatibility. For example, specifying version = "1.0.0".

  3. The pretty_env_logger dependency does not have a specific version specified. It's generally recommended to specify a specific version to ensure reproducibility. You can choose the latest stable version or specify a specific version based on your project requirements.

  4. There is no description of what the project does, and the [package] section is missing important metadata like author, license, and repository information. It's a good practice to provide these details to enhance project documentation and discoverability.

  5. The code patch related to bumping hyper version to 1.0 is missing. It's not possible to provide a review for the patch without the actual code changes.

Overall, make sure to specify specific versions for dependencies to ensure compatibility and provide necessary project metadata to improve documentation and discoverability.

The key changes in the patch are as follows:

  1. The hyper_wasi and tokio_wasi dependencies are removed.
  2. The hyper dependency is updated to version 1 and retains the features as "full".
  3. The tokio dependency is updated to version 1 and retains the features as "rt", "macros", "net", "time", and "io-util".
  4. The wasmedge_wasi_socket, pin-project, and http-body-util dependencies remain unchanged.

client/src/main.rs

Overall, the code looks well-structured and follows best practices. However, there are a few potential issues and improvements that can be addressed:

  1. Unused import: The code includes an unused import statement use tokio::io::{self, AsyncWriteExt as _};. It should be removed to keep the code clean.

  2. Error handling: The code uses unwrap in a few places, such as parsing the url_str into a hyper::Uri. While it may be acceptable for this specific example, it is generally better to handle errors explicitly using Result and provide meaningful error messages.

  3. Handling TCP stream: The code uses a TcpStream from the tokio::net module, but it first converts a wasmedge_wasi_socket::TcpStream into a raw file descriptor and then into the TcpStream. This conversion can be simplified by directly using tokio::net::TcpStream without the need for raw file descriptor conversion.

  4. Logging and debugging: The code currently uses println! for logging and debugging statements. Consider using a more robust logger, such as the log crate, which can provide different logging levels and output destinations.

  5. Commented code: There are a few lines of commented code that are not used anymore. These should be removed to avoid confusion.

  6. Code duplication: The code for fetching URLs is duplicated in the fetch_url and post_url_return_str functions. Consider refactoring this code into a shared helper function to avoid duplication.

  7. Error type: The MainResult type is defined as Result<T, Box<dyn std::error::Error + Send + Sync>>. While this is a valid choice, it is often clearer to define a custom error type specific to the application instead of using a generic Error trait object.

  8. Pin projection: The code uses the pin_project crate for pin projection. This crate allows pinning a struct and accessing fields in a pinned context. Make sure to understand and handle the intricacies of pinning correctly.

These are the potential issues and improvements. Remember to review the complete source code and consider its context for a more comprehensive review.

Key changes in the patch include:

  • Importing additional dependencies, including std::os::fd, pin_project, and http_body_util.
  • Changing the return type of the main function to use a custom error type MainResult.
  • Refactoring the fetch_url function to use a TCP stream and a TokioIo wrapper to handle reading and writing to the stream.
  • Implementing the Read and Write traits for the TokioIo type to allow it to be used with hyper's connection handling.
  • Updating the fetch_url function to use the new tokio::net::TcpStream type and the hyper::client::conn::http1::handshake function for establishing a connection.
  • Modifying the fetch_url function to use hyper's new sender and frame APIs for sending and receiving HTTP requests and responses.
  • Updating the post_url_return_str function to use the same connection handling approach as the fetch_url function.

server-tflite/Cargo.toml

The source code provided is not complete as it is a part of a configuration file (most likely a Cargo.toml file) rather than actual code. However, based on the information provided, here are some potential issues:

  1. Version Number: The code mentions bumping the hyper version to 1.0. However, the current version specified for the hyper dependency is just "1" without specifying a specific version. It is recommended to specify the exact version number when possible to ensure consistency and avoid potential compatibility issues.

  2. Deprecated Dependencies: The code includes the pin-project dependency with version "1.1.3". It is worth noting that the current version of pin-project is 1.0.9, and 1.1.3 might be deprecated or incompatible. It is recommended to check for the latest version compatible with the project.

  3. Deprecated Features: The code specifies deprecated features for the image dependency. It sets default-features to false while still specifying features, which are typically meant to be enabled through default-features. It is recommended to review the documentation for the specific image version being used and ensure the correct usage of features.

  4. Library Stability: The code does not indicate the stability guarantees of the dependencies used. It is crucial to ensure that stable and well-maintained libraries are used in a project to avoid potential issues and ensure long-term support.

Please note that without complete source code, it is difficult to provide a comprehensive review. The issues mentioned above are based on the provided information and may not reflect the actual implementation. It is advised to review the entire codebase and consider following best practices for dependency management and versioning in Rust.

The key changes in the patch are as follows:

  1. hyper_wasi dependency has been replaced with hyper dependency with version "1" and the "full" feature enabled. This change bumps the version of hyper being used.

  2. tokio_wasi dependency has been replaced with tokio dependency with version "1" and the features "rt", "macros", "net", "time", and "io-util" enabled. This change bumps the version of tokio being used.

  3. wasmedge_wasi_socket dependency has been added with version "0.5".

  4. The features for the image dependency have been reformatted and moved to separate lines for improved readability.

  5. pin-project, http-body-util, and bytes dependencies have been added with version "1.1.3", "0.1.0", and "1" respectively.

Overall, the patch updates the dependencies used, bumps the version of hyper and tokio, adds a new dependency, and makes changes to improve readability.

server-tflite/src/main.rs

Reviewing the provided source code, I have identified a few potential problems:

  1. In the classify function, the model_data and labels variables are defined as slices (&[u8] and &str respectively), but they are assigned values using the include_bytes! and include_str! macros. These macros return byte arrays and string literals respectively. This might result in a type mismatch error at compile time. It is recommended to define model_data as &[u8; N] and labels as &'static str where N is the length of the byte array.

  2. The code includes commented out sections that use the wasi_nn crate for loading and executing the neural network model. These sections are currently replaced with code that uses the GraphBuilder and GraphEncoding structs. It is not clear if this replacement is intentional or if it was only meant for testing purposes. If the wasi_nn crate is the proper dependency, the commented out code should be removed or replaced with the correct usage of the wasi_nn crate.

  3. The sort_results function sorts the buffer of probabilities, but it assumes that the graph places the match probability for each class at the index for that class in the buffer. This assumption might not be valid in all cases. It would be better to have a clear understanding of the graph's output format and handle that accordingly.

  4. The main function uses the unsafe keyword when creating a TcpListener from a raw file descriptor. This usage of unsafe requires careful consideration to ensure that all the necessary safety guarantees are upheld. It is recommended to add a comment explaining why the unsafe block is necessary and detailing the safety precautions taken.

  5. The main function has a loop that accepts incoming connections and spawns tasks to handle them. However, there is no mechanism to gracefully shut down the server. It would be beneficial to add a signal handler to capture SIGTERM or SIGINT signals and initiate a graceful shutdown.

Overall, the code functionality looks fine, but the above points should be addressed to improve code quality and ensure correctness.

The key changes in the patch include:

  • Importing additional modules and structs: bytes::Bytes, http_body_util::{combinators::BoxBody, BodyExt, Full}, hyper::server::conn::http1, pin_project::pin_project, std::os::fd::{FromRawFd, IntoRawFd}, std::pin::Pin, std::task::{Context, Poll}, tokio::net::TcpListener
  • Changing the function signature of classify to accept a Request<hyper::body::Incoming> and return a Result<Response<BoxBody<Bytes, hyper::Error>>, anyhow::Error>
  • Modifying the implementation of the classify function to use req.collect().await?.to_bytes() instead of hyper::body::to_bytes(req.into_body()).await?
  • Adding a new struct TokioIo and implementing hyper::rt::Read and hyper::rt::Write traits for it
  • Adding a new helper function full<T: Into<Bytes>>(chunk: T) -> BoxBody<Bytes, hyper::Error>
  • Modifying the main function to use TcpListener from the wasmedge_wasi_socket crate instead of Server::bind from the hyper crate
  • Adding a loop to accept incoming connections using listener.accept().await? and serve them with http1::Builder

server/Cargo.toml

Overall, the code appears to be fine. However, there are a few potential problems:

  1. Outdated dependencies: The hyper dependency should be bumped to version 1.0, as mentioned in the subject. Currently, it is set to version 1 without specifying the exact version (e.g., 1.0). Make sure to update this dependency accordingly.

  2. Unused dependencies: There are a few dependencies specified (pin-project, http-body-util, bytes) that are not being used in the source code. It is recommended to remove these dependencies to improve code clarity and reduce unnecessary dependencies.

  3. Inconsistent dependency format: The wasmedge_wasi_socket dependency is specified with a different format than the other dependencies. Consider following the same format as the other dependencies ({ name = "dependency_name", version = "version_number" }).

After addressing these potential problems, the code should be in a better state.

The key changes in the patch are as follows:

  1. Updated the version of the hyper_wasi dependency to 1 and added the features field with the value ["full"].
  2. Updated the version of the tokio_wasi dependency to 1 and added the features field with the value ["rt", "macros", "net", "time", "io-util"].
  3. Added the wasmedge_wasi_socket dependency with version "0.5".
  4. Added new dependencies: pin-project with version "1.1.3", http-body-util with version "0.1.0", and bytes with version "1".

server/src/main.rs

Overall, the code appears to be well-structured and follows good coding practices. However, there are a few potential issues that can be addressed:

  1. Error Handling: The code does not have proper error handling for various operations. For example, in the main function, the results of bind, from_raw_fd, and from_std should be checked for errors. Additionally, in the echo function, some operations may fail and return an Err, so those errors should also be properly handled.

  2. API Compatibility: The code uses several features and APIs from different versions of the hyper and tokio libraries. It's important to ensure that the version bumps do not cause any compatibility issues.

  3. Memory Management: The code uses Bytes, which represents a non-owning reference to a byte array, for handling HTTP request/response bodies. This is generally efficient, but it's important to ensure that memory is properly managed when dealing with larger payloads.

  4. Code Organization: The code could benefit from some refactoring and organizing to improve clarity and maintainability. For example, splitting the code into separate modules or functions based on their responsibilities would make it easier to understand and modify in the future.

  5. Code Comments: The code would benefit from more comments to explain the purpose and functionality of each component, especially for the custom TokioIo struct and its implementation.

Please note that these are general recommendations based on the provided code snippet. Further analysis may be required to determine if there are any other issues or improvements specific to the complete codebase.

Key changes in the patch include:

  • Importing new dependencies (std::os::fd::{FromRawFd, IntoRawFd}, std::pin::Pin, std::task::{Context, Poll}, http_body_util::{combinators::BoxBody, BodyExt, Empty, Full}, pin_project::pin_project).
  • Defining a new struct TokioIo implementing the hyper::rt::Read and hyper::rt::Write traits.
  • Implementing required trait methods for TokioIo.
  • Modifying the echo function signature to now take Request<hyper::body::Incoming> and return Result<Response<BoxBody<Bytes, hyper::Error>>, hyper::Error>.
  • Modifying the response body construction in the echo function to use the full and empty helper functions.
  • Adding new endpoint implementations for /echo/uppercase and /echo/reversed.
  • Adding helper functions empty and full for constructing BoxBody instances.
  • Modifying the listening address to use 127.0.0.1:3000.
  • Binding the TcpListener using wasmedge_wasi_socket::TcpListener and converting it to a TcpListener from tokio::net::TcpListener.
  • Modifying the server connection handling to use http1::Builder and serve_connection instead of Http and serve_connection.
  • Adding debug print statement for accept.

@L-jasmine L-jasmine force-pushed the bump/hyper1.0 branch 3 times, most recently from 6edb00a to 41e676b Compare January 22, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants