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

refactor: Coroutine based webserver #1699

Merged
merged 54 commits into from
Oct 24, 2024

Conversation

kuznetsss
Copy link
Collaborator

Code of new coroutine-based web server. The new server is not connected to Clio and not ready to use yet.
For #919.

@kuznetsss kuznetsss force-pushed the 919_Refactor_webserver branch from a611c3c to 633707a Compare October 21, 2024 15:24
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

Attention: Patch coverage is 79.74684% with 112 lines in your changes missing coverage. Please review.

Project coverage is 71.67%. Comparing base (cf081e7) to head (a7a40d4).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/web/ng/Server.cpp 72.32% 31 Missing and 13 partials ⚠️
src/web/ng/impl/HttpConnection.hpp 56.25% 13 Missing and 15 partials ⚠️
src/web/ng/impl/WsConnection.hpp 72.72% 11 Missing and 1 partial ⚠️
src/web/ng/impl/ConnectionHandler.cpp 89.81% 4 Missing and 7 partials ⚠️
src/web/ng/impl/WsConnection.cpp 37.50% 9 Missing and 1 partial ⚠️
src/web/ng/Response.cpp 91.66% 2 Missing and 1 partial ⚠️
src/web/ng/Request.cpp 95.45% 1 Missing and 1 partial ⚠️
src/util/WithTimeout.hpp 93.33% 0 Missing and 1 partial ⚠️
src/web/Server.hpp 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1699      +/-   ##
===========================================
+ Coverage    71.30%   71.67%   +0.37%     
===========================================
  Files          266      276      +10     
  Lines        11143    11655     +512     
  Branches      5710     5929     +219     
===========================================
+ Hits          7945     8354     +409     
- Misses        1669     1730      +61     
- Partials      1529     1571      +42     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@godexsoft godexsoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks great. It's a huge improvement over the current webserver implementation.
I left a few questions and suggestions. Also, feel free to sprinkle [[nodiscard]] where it makes sense and maybe noexcept too.

src/web/impl/AdminVerificationStrategy.cpp Show resolved Hide resolved
src/web/ng/Connection.hpp Show resolved Hide resolved
src/web/ng/Request.cpp Outdated Show resolved Hide resolved
src/web/ng/Request.hpp Outdated Show resolved Hide resolved
src/web/ng/Request.hpp Show resolved Hide resolved
src/web/ng/Server.cpp Show resolved Hide resolved
src/web/ng/impl/Concepts.hpp Outdated Show resolved Hide resolved
tests/common/util/TestHttpClient.hpp Show resolved Hide resolved
tests/common/util/TestHttpClient.cpp Outdated Show resolved Hide resolved
tests/common/util/TestWebSocketClient.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@cindyyan317 cindyyan317 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 Nice ! I just leave some questions.

@kuznetsss kuznetsss merged commit cffda52 into XRPLF:develop Oct 24, 2024
17 checks passed
godexsoft pushed a commit that referenced this pull request Nov 11, 2024
Code of new coroutine-based web server. The new server is not connected
to Clio and not ready to use yet.
For #919.
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.

3 participants