Development/messageunit

[msieben]

No description provided.

[msieben]

@VeithMetro you might start at cdc50b2 for the changes in the core files.

[VeithMetro]

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

[msieben]

It actually caused a pure virtual function call fault - at the time of writing. See cdc50b2

[VeithMetro]

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

[VeithMetro]

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 the std::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?

[msieben]

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.

[VeithMetro]

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?

[VeithMetro]

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?

[msieben]

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.

[msieben]

See cdc50b2

[VeithMetro]

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

[msieben]

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?

[VeithMetro]

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?

[msieben]

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);

) but is never revoked (

Thunder/Source/core/MessageStore.h

Line 136 in 200249e

static void Revoke(IControl* control);

) would trigger a segfault at program end (

Thunder/Source/core/MessageStore.cpp

Line 57 in 200249e

_controlList.front()->Destroy();

) or via a call to Controls::Iterate for a handler (

Thunder/Source/core/MessageStore.cpp

Line 93 in 200249e

void Iterate(Core::Messaging::IControl::IHandler& handler)

) calling a method on it if it already went out of scope.

[VeithMetro]

Agreed, that's why in messaging we make sure that a control will always be Revoked() (here) thus removed from our _controlList

[msieben]

@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.

[VeithMetro]

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 IControl, but didn't include either Announce() in its constructor, nor Revoke() in its destructor. Then I created an object of this custom Control class, and announced it via Core::Messaging::IControl::Announce(). Since it has not been revoked at all after its destruction, it became an invalid pointer in the ControlList, and shortly after caused a crash on Thunder's side, so exactly as expected.

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 Revoke(), same as if he did an AddRef() without a Release()), we shouldn't try to defend him from that. There are many even simpler way to crush Thunder if you try really hard, but defending from stuff like that means compensating efficiency, and I don't think we want that 😄

[msieben]

@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?

[pwielders]

@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..

[pwielders]

Oops did not want to close the PR :-)