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

Add rudimentary support for websocket max payload #425

Conversation

DeuSCaNoN
Copy link
Contributor

Implementation for issue #421

@The-EDev The-EDev linked an issue Apr 27, 2022 that may be closed by this pull request
if (remaining_length_ > max_payload_bytes_)
{
close_connection_ = true;
adaptor_.close();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This close() call will remove the ability to call get_remote_ip() in the onclose() call the user has customized.

I'm unsure if we have to close the adapter first. Can we error_handler_() and check_destroy() first then close the adaptor?

Copy link
Member

@The-EDev The-EDev Apr 28, 2022

Choose a reason for hiding this comment

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

This is a completely different issue to be honest. What I would do is copy the ip address to the websocket connection class as soon as the adaptor is available. That way closing the adaptor won't affect the connection object in any way.

Copy link
Member

Choose a reason for hiding this comment

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

The Idea with close and error handlers is that they're meant to do any required cleanup without sending or receiving data.

@crow-clang-format
Copy link

--- include/crow/websocket.h	(before formatting)
+++ include/crow/websocket.h	(after formatting)
@@ -395,7 +395,7 @@
                             close_connection_ = true;
                             adaptor_.close();
                             if (error_handler_)
-                              error_handler_(*this);
+                                error_handler_(*this);
                             check_destroy();
                         }
                         else if (has_mask_)

@The-EDev The-EDev changed the title #421 add rudimentary support for websocket max payload Add rudimentary support for websocket max payload Apr 27, 2022
@The-EDev
Copy link
Member

@DeuSCaNoN Sorry for the delat, #432 just got merged, it adds support for passing the Crow object (app) to a websocket connection, could you take a look at how it can be used in this PR? Thanks a lot!

@The-EDev The-EDev force-pushed the #421-Websockets-should-support-a-maximum-payload branch from a218db3 to dc988ce Compare May 14, 2022 11:07
include/crow/routing.h Outdated Show resolved Hide resolved
@crow-clang-format
Copy link

--- include/crow/app.h	(before formatting)
+++ include/crow/app.h	(after formatting)
@@ -474,7 +474,7 @@
         std::uint8_t timeout_{5};
         uint16_t port_ = 80;
         uint16_t concurrency_ = 2;
-        uint64_t max_payload_{ UINT64_MAX };
+        uint64_t max_payload_{UINT64_MAX};
         bool validated_ = false;
         std::string server_name_ = std::string("Crow/") + VERSION;
         std::string bindaddr_ = "0.0.0.0";

include/crow/routing.h Outdated Show resolved Hide resolved
@The-EDev
Copy link
Member

On a quick glance everything seems alright, but I'll take a more thorough look in the afternoon/evening.

Thanks a lot for the work you've done @DeuSCaNoN (and thanks for putting up with my comments 😅).

@The-EDev
Copy link
Member

Code looks alright, I was going to ask you to remove set_max_payload_size() but then I realized it could be useful for changing the max payload size per-connection on runtime.
The only thing left is for me to add a unit test, include the code in an example, and maybe write a bit of documentation.

once again thank you very much for your work on this, I can't wait to merge it.

@The-EDev
Copy link
Member

I've implemented a simple test and it seems to be failing, aside from a few errors I found it seems the code is simply allowing payloads larger than the limit. I'll have to investigate this further.

@The-EDev
Copy link
Member

@DeuSCaNoN I added small fixes, a unit test, and documentation. Could you please take a look at them and make sure everything's okay?

@crow-clang-format
Copy link

--- tests/unittest.cpp	(before formatting)
+++ tests/unittest.cpp	(after formatting)
@@ -2283,9 +2283,9 @@
 
     CROW_WEBSOCKET_ROUTE(app, "/ws")
       .onopen([&](websocket::connection&) {
-                                        connected = true;
-                                        CROW_LOG_INFO << "Connected websocket and value is " << connected;
-                                    })
+          connected = true;
+          CROW_LOG_INFO << "Connected websocket and value is " << connected;
+      })
       .onmessage([&](websocket::connection& conn, const std::string& message, bool isbin) {
           CROW_LOG_INFO << "Message is \"" << message << '\"';
           if (!isbin && message == "PINGME")

@The-EDev
Copy link
Member

It seems like something broke after #269 was merged...

@The-EDev The-EDev force-pushed the #421-Websockets-should-support-a-maximum-payload branch from b2ec3a6 to f2b63f2 Compare May 20, 2022 11:15
@The-EDev The-EDev merged commit 8bc0a4d into CrowCpp:master May 20, 2022
@DeuSCaNoN
Copy link
Contributor Author

:)

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.

Websockets should support a maximum payload
2 participants