-
Notifications
You must be signed in to change notification settings - Fork 127
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
Smartlink crash fix #1819
Smartlink crash fix #1819
Conversation
(cherry picked from commit 7e8c1a7bacd59f3f3169b524b9928f3dd3111218)
@@ -1174,6 +1225,21 @@ namespace Thunder { | |||
class Connection : public LinkType<INTERFACE> { | |||
private: | |||
using Base = LinkType<INTERFACE>; | |||
class EventSubscriber: public Core::Thread { |
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.
Do we really need a thread here ? This will always (per connection) require an 8MB callstack. I thnk you can "reuse" the ResouceMonitor Thread (communication thread) as this is probably just to request a registration ? Or if we need a thread, could we do something to limit it to 1 thread for all Conenction types per process ?
uint32_t retVal = Core::ERROR_UNAVAILABLE; | ||
Core::JSONRPC::Message response; | ||
const string parameters("{ \"event\": \"" + eventName + "\", \"id\": \"" + Base::Namespace() + "\"}"); | ||
auto result = Base::template Invoke<string>(DefaultWaitTime, "register", parameters, response); |
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.
Oke, synchronous Invoke, so communication thread is not an option :-) Maybe than 1 thread for all processes that have this ?
No description provided.