Handle request atomically #249
Conversation
Codecov Report
@@ Coverage Diff @@
## master #249 +/- ##
===========================================
- Coverage 63.83% 53.38% -10.46%
===========================================
Files 94 69 -25
Lines 5649 2424 -3225
Branches 1807 1087 -720
===========================================
- Hits 3606 1294 -2312
+ Misses 973 432 -541
+ Partials 1070 698 -372
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 25 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
estringana
force-pushed
the
estringana/handle-request-atomicaly
branch
2 times, most recently
from
f14c17a
to
1680666
Compare
estringana
force-pushed
the
estringana/handle-request-atomicaly
branch
from
1680666
to
31f0e71
Compare
| return false; | ||
| static constexpr auto client_init_timeout{std::chrono::milliseconds{500}}; | ||
| return handle_message<network::client_init>( | ||
| *this, *broker_, client_init_timeout, false); |
There was a problem hiding this comment.
Why would you want to ignore unexpected messages here?
There was a problem hiding this comment.
Basically what happen is that client_init will handle unexpected messages differently from the rest. Client_init will disconnect client(according to existing tests added by you) while the rest of command won't. Since you asked to move the handling of unexpected commands to handle_message, somehow I need to let it know what to do when unexpected_command exception is thrown
There was a problem hiding this comment.
I completely read this the other way around for some reason, I guess that's fine then.
| @@ -73,7 +67,7 @@ bool send_error_response(const network::base_broker &broker) | |||
| template <typename... Ms> | |||
| // NOLINTNEXTLINE(google-runtime-references) | |||
| bool handle_message(client &client, const network::base_broker &broker, | |||
| std::chrono::milliseconds initial_timeout) | |||
| std::chrono::milliseconds initial_timeout, bool ignore_unexpected_messages) | |||
There was a problem hiding this comment.
This is textbook boolean-trap :-(
| } | ||
|
|
||
| return true; | ||
| // TODO: figure out how to handle errors which require sending an error |
There was a problem hiding this comment.
You can probably remove this TODO now...
Description
Handle request atomically
Motivation
Additional Notes
Describe how to test your changes
Readiness checklist
Release checklist