fix: DIOS-7656 AsyncSafe might be called when the object is in destruction or construction #390
Conversation
…tion. No need to assert in that case.
|
If the object is deleted, then wouldn't the |
harryz2000
changed the title
The object's OnRTP shouldn't be referenced if it is after destruction, in that case the behaviour would be undefined. This fix only works when the OnRTP was called during the destruction where initialized is still valid in destructor. If the object is already freed. Both the timeservice and initialized would be invalid. We have to prevent that from happening. I don't see that could happen yet. As we should be calling Stop() first and then release the object. After another thinking, to be safer, I would add destruction code in RTPIncomingMediaStreamMultiplexer class to call stop() during destruction. That should help to avoid this issue. |
| void Stop(); | ||
| private: | ||
| RTPIncomingMediaStream::shared incomingMediaStream; | ||
| std::set<RTPIncomingMediaStream::Listener*> listeners; | ||
| volatile bool muted = false; | ||
|
|
||
| bool stopped = false; |
There was a problem hiding this comment.
Can we set initialized = false instead of a second bool? Will also assert/protect against using the object in async calls after we have stopped it.
The stop currently assumes that it is the last thing executed.
There was a problem hiding this comment.
I understand two bools seems duplicated. However, I am keen to separate the logic in the concrete class from the base TimeServiceWrapper class. The initialized was created for check whether it is constructed to avoid misuse of TimeServiceWrapper. I feel it would make it complicated to expose it to child classes.
I am current investigating a destruction issue related to this. I would do a few more tweaks around that. Will resend it for review once it's done.
…object is stopped
|
I further tweaked the TimeServiceWrapper to make it has a state to indicate when the Sync/Async could be called. It is found in some cases, the dependency such a TimeService passed to MediaFrameListenerBridge might be freed before MediaFrameListenerBridge is deallocated. In that case, whenever the timeService is used, it would cause segment failure. The new added state "Stopped" would prevent this happening by not allow to use timeService after this state. I also keep the stopped member in the concrete class. I feel that make it clearer as it separates the class logic and the TimerServiceWrapper implementation. I changed it to a atomic to make it more robust. I would check other classes that using TimeServiceWrapper as similar classes should also needed to have this "Stopped" handling. But that could be in a separate PR though. |
|
I still don't understand the rationale of this PR. What issue are we trying to solve? can you put a concrete example? |
Sure. Yes. It is a bit tricky. I will create a more detailed description about this change shortly. |
include/TimeService.h
Outdated
| obj->OnCreated(); | ||
| return obj; | ||
| } | ||
|
|
||
|
|
||
| TimeServiceWrapper(TimeService& timeService) : timeService(timeService) | ||
| { | ||
| SetTimeServiceWrapperState(WrapperState::Destructed); |
There was a problem hiding this comment.
initial state shouldn't be Destructed
There was a problem hiding this comment.
The was wrong indeed.
| @@ -141,7 +141,7 @@ class MediaFrameListenerBridge : | |||
| std::chrono::milliseconds dispatchingDelayMs = std::chrono::milliseconds(0); | |||
| std::chrono::milliseconds maxDispatchingDelayMs = std::chrono::milliseconds(MaxDispatchingDelayMs); | |||
|
|
|||
| bool stopped = false; | |||
| std::atomic_bool stopped { false }; | |||
There was a problem hiding this comment.
do we need this atomic? we can set the stopped flag inisde the Sync scope instead
There was a problem hiding this comment.
Using flag in Sync call could cause the Sync call be reentered if the previous Stop() is not finished. i.e.
if (stopped) return;
<-------- If another Stop() call happens here, the following code would be executed twice.
Sync({
....
stopped = true;
});
But if we use atomic_bool, as it set stopped immediately, it would guarantee the Sync() code wouldn't be called twice.
include/TimeService.h
Outdated
| Uninitialized, | ||
| Initialized, | ||
| Stopped, | ||
| Destructed |
There was a problem hiding this comment.
I agree. It is only to distinguish with other states. I am OK to remove.
| Destructed | ||
| }; | ||
|
|
||
| inline void SetTimeServiceWrapperState(WrapperState timeServiceWrapperState) |
There was a problem hiding this comment.
we don't want the caller code to change the state to a wrong state, can we make it private? Not sure if it could be called from the factory method.
Should be better to add a
inline void Stop()
{
SetState(Stopped);
}
There was a problem hiding this comment.
Yes. I thought of that. It is just I find using Stop() might have name conflict with existing code. I may create a StopTimeServiceWrapper() function for that.
Due to race condition, the object passing to AsyncSafe might be deleted. This fix is to avoid assertion when if it is expired after construction. We still assert to ensure the object is constructed to avoid misuse, for example. calling AsyncSafe from constructor.