-
Notifications
You must be signed in to change notification settings - Fork 128
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
Development/messageunit #1714
base: master
Are you sure you want to change the base?
Development/messageunit #1714
Conversation
Includes a workaround for a unresponsive child process.
…PTestAdministrator'
…cb61d929c' Including various improvements for some test.
…turnLastPushedMessageInOtherProcess' in test_message_unit'
…e' in 'test_message_unit'
…ditionally annnounce ourself
…we unconditionally annnounce ourself" This reverts commit 5248a4b.
…bility of the user of 'Announce' - Avoid pure virtual call - MessageUnit should not clean up / destroy (I)Controls announced via the SYSLOG_ANNOUNCE, OPERATIONAL_STREAM_ANNOUNCE and ANNOUNCE_WARNING
df1a260
to
90f3519
Compare
…7c26374062ec9e07c84a49bf9c43' may not be used
251656f
to
c7e259c
Compare
c7e259c
to
b4f620e
Compare
@VeithMetro you might start at cdc50b2 for the changes in the core files. |
TRACE_L1(_T("MessageControl %s, size = %u was not disposed before"), typeid(control).name(), static_cast<uint32_t>(_controlList.size())); | ||
_controlList.front()->Destroy(); |
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 are we dropping Destroy()
here?
I tried to produce a scenario in which the _controlList
was not clearly properly and this TRACE_L1
was shown, and I couldn't get to that point, but I think at least in theory it is still possible, right?
In that case, I think that the removal of Destroy()
might lead to a segmentation fault as this destructor is the consequence of MessageUnit being destructed, thus messages should no longer be sent
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.
It actually caused a pure virtual function call fault - at the time of writing. See cdc50b2
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.
Here is a similar situation to #1714 (comment). If we destory the whole Controls class which stores all IControl
objects in ControlList
, then it feels necessary to notify the other side about this fact, to make sure it won't try to perform any more traces, syslogs, etc. on these controls
|
||
if (bufferSize > (sizeof(_type) + (sizeof(_category[0]) * 2))) { | ||
if (bufferSize > (sizeof(_type) + _category.size() + 1)) { |
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.
Correct me if I am wrong but as far as I understand, in the Deserialize()
we expect the _category
to be an empty string, so _category.size()
returns zero, right?
Before we had (sizeof(_category[0]) * 2))
, so:
_category[0]
refers to the first character of thestd::string
object- If the string is empty, the
operator[]
should still return a char with the value\0
(null terminator) sizeof(_category[0])
is the size of the character type (char), which is always 1 byte- and for some bizarre reason we were multiplying it by two, probably because we wanted to make sure the buffer holds at least one character besides a null terminator?
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.
I cannot correct you because I do not know the answer. I am not the original author of the code. What I noticed was that the test failed and length in Serialize() is given by https://github.com/rdkcentral/Thunder/blob/development/messageunit/Source/core/MessageStore.cpp#L130.
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.
I see, what I think is that _category.size()
is not necessary here since it should always return 0 in the Deserialize()
- perhaps you could check this in tests.
Instead of using it, maybe it would be better to just check if bufferSize > (sizeof(_type) + 1)
if we want to potentially allow empty categories, or + 2
if we want to make sure there is at least one character..
On the other hand, perhaps it would be even better to only keep the ASSERT and remove this condition altogether @sebaszm?
@@ -312,7 +314,6 @@ namespace Thunder { | |||
Messaging::ConsoleStandardOut::Instance().Close(); | |||
} | |||
Core::Messaging::IStore::Set(nullptr); | |||
Core::Messaging::IControl::Iterate(handler); |
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.
Here it also looks like we are getting rid of a Destroy()
call via iterate on all handles. Basically the whole section above with the Handler
class definition and its Handle()
method calling Destroy()
is no longer used, why is that?
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.
You might recall the discussion outside the Github context about the ANNOUNCE-macros and elements initially added do not re-appear on open-close-open of MessageUnit.
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.
See cdc50b2
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.
I see, but it looks to me that if we drop this Iterate() and thus Destroy(), the other side (e.g. here) will not be aware that MessageUnit has been closed and its controls should no longer be used
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.
You (@VeithMetro) mentioned the use case is not what had been considered in the original design. However, from the tests it can be concluded that is a defect iff allowed. Imho allowing an explicit Close (https://github.com/rdkcentral/Thunder/blob/development/messageunit/Source/messaging/MessageUnit.h#L1007) suggests the code flow is allowed. A more recent test illustrates no current issue by re-adding the Destroy(). However, the code is not reached in any of the current tests (anymore). I suggest for now it is best to revert the removal of Destroy() and see what the future brings, or, try to implement a test which deliberately can excite that code point. What is your opinion?
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.
I think having a test which deliberately can excite that point in code, and which preferably suits a real use case would be great. Have you maybe tried to run Thunder to see if the Close() is ever executed? E.g. when closing MessageControl or other plugins which use messaging?
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.
Any object that derives from IControl that is passed via Announce (
Thunder/Source/core/MessageStore.h
Line 135 in 200249e
static void Announce(IControl* control); |
Thunder/Source/core/MessageStore.h
Line 136 in 200249e
static void Revoke(IControl* control); |
Thunder/Source/core/MessageStore.cpp
Line 57 in 200249e
_controlList.front()->Destroy(); |
Thunder/Source/core/MessageStore.cpp
Line 93 in 200249e
void Iterate(Core::Messaging::IControl::IHandler& handler) |
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.
Agreed, that's why in messaging we make sure that a control will always be Revoked() (here) thus removed from our _controlList
@pwielders After having an offline discussion with @VeithMetro we must conclude that there are some architectural questions about the use of asserts and conditions tests present in Serialize() and Deserialize here and in the master branch. Moreover, the interface allows use cases for which it was possibly not intended. I invite you to continue the conversation offline. |
Earlier today I managed to reproduce two similar scenarios, which both led to a segmentation fault in Thunder, one for in process plugin and the other for out of process: I created a custom Control class based on After that, I talked about it extensively with @sebaszm, and we both came to the conclusion that if a user does bad programming and he tries to shoot himself in the foot on purpose (not including the |
@VeithMetro sounds like similar scenarios I have shared with you offline ( #1749). I guess it is all about tradeoffs and intent. Question is: should users derive from IControl outside core? That question arose in our discussion but remains unanswered. @pwielders can you comment on that? |
@msieben / @VeithMetro We try to be as lean and mean as possible. We expect a certain maturity of developers that write code for an embedded system as trivial as the new and delete, as trivial should an announce and a revoke be. This is what we call interface symmetry. If you allocate, you should free. If you Announce, you should Revoke. If you forget t do the Announce, the Trace will not work, If you forget to Revoke if you are done with that module and the code crashes it is like creating a memory leak and thus we call it a "bug" and it must be solved. If we could ASSERT in such a situation, it is good. But lets not spend more resources on trying to recover from programming mistakes. For he questions: "Should users derive from IControl outside core and or messaging, the answer is NO. The IController is used internally by the messaging system and should not be used outside of the Messaging scope. The users of messaging can create custom TRACES but for that they do not need to inherit from IController. So a test case that deliberately does an Announce but does not a revoke, is like a test case where you would do New(), not a delete and report you are leaking that a bit of a given and for me it means you need to fix the test case and not the allocation of the OS ;-) I think the smae is applicable for this Announce/Revoke.. |
Oops did not want to close the PR :-) |
No description provided.