-
Notifications
You must be signed in to change notification settings - Fork 68
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
Server code improvements #250
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -38,6 +38,29 @@ | |||
} \ | |||
} | |||
|
|||
namespace { | |||
|
|||
constexpr uint32_t Integer(const std::string_view str) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function should probably be named StringHash or something that actually explains what it does.
Is there any compile time guarantee that the strings we use it with actually hash to different values? I guess we would hope to get a compile error if it resulted in a switch with multiple identical case
s?
edit: but I see we are not using switch..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed it and changed it back to a switch
} catch (...) { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do nothing here? Seems like we should log an error if possible?
default: { | ||
try { | ||
const auto intOp = Integer(op); | ||
if (intOp == SUBSCRIBE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be a switch?
inline bool Server<ServerConfiguration>::hasHandler(uint32_t op) const { | ||
switch (op) { | ||
case SUBSCRIBE: | ||
return static_cast<bool>(_handlers.subscribeHandler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you can just do bool(...)
### Public-Facing Changes Fix segfault due to invalid iterator ### Description Fixes a segfault caused by using an invalid iterator as argument to `std::unordered_map<...>::erase`. This regression was introduced in #250.
### Public-Facing Changes Fix specific exceptions not being caught ### Description Fixes specific exceptions not being caught, leading to vague error messages. This regression got introduced with #250.
Public-Facing Changes
Refactor server code, improve multi-threading safety
Description
Addresses comments from foxglove/ws-protocol#491. These changes will be applied to that PR once this PR gets merged.