paritytech / jsonrpsee

Rust JSON-RPC library on top of async/await
MIT License
629 stars 169 forks source link

upgrade to hyper v1.0 #1368

Closed niklasad1 closed 4 months ago

niklasad1 commented 4 months ago

Close #1257

Things that could be improved in this PR or in follow-up:

niklasad1 commented 4 months ago

@CodiumAI-Agent /review

CodiumAI-Agent commented 4 months ago

PR Review 🔍

⏱️ Estimated effort to review [1-5] 5, because the PR involves multiple files with significant changes including feature updates, refactoring, and dependency updates. The complexity and breadth of the changes require a thorough review to ensure compatibility, performance, and adherence to best practices.
🧪 Relevant tests No
⚡ Possible issues Possible Bug: The PR introduces changes across multiple files which could lead to integration issues if not properly tested. For example, updates in dependency versions and major refactoring could affect existing functionalities.
Performance Concern: The extensive use of new features and libraries might impact the performance. It is crucial to benchmark the new changes against the current implementation to ensure there is no performance degradation.
🔒 Security concerns No
Code feedback:
relevant fileserver/src/server.rs
suggestion       Consider adding integration tests to cover the new changes introduced in the server configuration and connection handling. This is important to ensure that the new configurations and the refactored connection handling work as expected under different scenarios. [important]
relevant lineuse hyper::body::Bytes;

relevant fileserver/src/server.rs
suggestion       It's recommended to handle possible errors from the `tokio::net::TcpListener::bind` method to prevent the server from panicking at runtime if the address is already in use or if there are insufficient permissions to bind to the address. [important]
relevant linelet listener = tokio::net::TcpListener::bind(SocketAddr::from(([127, 0, 0, 1], 0))).await.unwrap();

relevant fileserver/src/server.rs
suggestion       For better error handling and to avoid potential data loss, consider implementing a backpressure mechanism or a way to handle backpressure from clients in the server implementation. This could prevent the server from being overwhelmed by too many requests or data at once. [medium]
relevant lineuse futures_util::future::{self, Either};

relevant fileserver/src/server.rs
suggestion       To improve the modularity and readability of the server setup code, consider refactoring the large async block inside `tokio::spawn` into separate functions or methods. This would make the code easier to maintain and test. [medium]
relevant linetokio::spawn(async move {