Critical section begin end() changing the auto value
Hello,
In the following piece of code, used to work. Now i was debugging something else (in debug mode) and i see an issue:
Code:
const auto& sDecouplingId = ulDlDecoupcells.m_bIsMultiplePair ? "NR cell decoupled more than once" : cpTddCell->GetID();
const auto& bEnableDecoupling = true;
pTddCopyCellCarrier->SetDecouplingId(sDecouplingId);
pTddCopyCellCarrier->SetDecouplingEnabled(bEnableDecoupling);
EnterCriticalSection(&m_CriticalSectionDB);
if (!BusinessObjects::BO_DBHelper::instance().Apply(pTddCopyCell.get(), pTddCopyCell.get()))
{
AI_Message::instance()->Add("Database Error. Failed to update cell: %s", pTddCopyCell->GetID());
LeaveCriticalSection(&m_CriticalSectionDB);
return;
}
LeaveCriticalSection(&m_CriticalSectionDB);
pSulCopyCellCarrier->SetDecouplingId(sDecouplingId);
pSulCopyCellCarrier->SetDecouplingEnabled(bEnableDecoupling);
The local variable "sDecouplingId " is having some garbage value after the LeaveCriticalSection() function !!!.
As it is release is closing, i found a shortcut and found and did the following change to it work
Code:
pSulCopyCellCarrier->SetDecouplingId(pTddCopyCellCarrier->GetDecouplingId());
pSulCopyCellCarrier->SetDecouplingEnabled(bEnableDecoupling);
I want to know, why the earlier code started to behave like that ?
I have not run in Release mode . Is it because of the debug mode ?
thanks
Pdk
Re: Critical section begin end() changing the auto value
sDecouplingId is a ref - so to what it references has to be valid throughout its life. This is obviously multi-threaded code - so I suspect that the problem is not related to the critical section (as this doesn't use cpTddCell) but to another thread which is changing cpTddCell between where sDecouplingId is set and where it is used. This is an issue when ref variables are used in multi-threading code. It may seem to work at some points - but it's not thread-safe code.
If critical section is already in use, then this thread will be suspended until it is available. During this period, another thread could be changing cpTddCell.
Depending,
Code:
const auto& sDecouplingId = ulDlDecoupcells.m_bIsMultiplePair ? "NR cell decoupled more than once" : cpTddCell->GetID();
const auto& bEnableDecoupling = true;
pTddCopyCellCarrier->SetDecouplingId(sDecouplingId);
the SetDecouplingId() could also be incorrect here if cpTddCell() is changed between where sDecouplingId is set and when it is used.
Re: Critical section begin end() changing the auto value
Thanks a lot kaud. cpTddCell is not changed in the other thread (I think *)..
Code:
EnterCriticalSection(&m_CriticalSectionDB);
const auto& sDecouplingId = ulDlDecoupcells.m_bIsMultiplePair ? "NR cell decoupled more than once" : cpTddCell->GetID();
const auto& bEnableDecoupling = true;
pTddCopyCellCarrier->SetDecouplingId(sDecouplingId);
pTddCopyCellCarrier->SetDecouplingEnabled(bEnableDecoupling);
if (!BusinessObjects::BO_DBHelper::instance().Apply(pTddCopyCell.get(), pTddCopyCell.get()))
{
AI_Message::instance()->Add("Database Error. Failed to update cell: %s", pTddCopyCell->GetID());
LeaveCriticalSection(&m_CriticalSectionDB);
return;
}
pSulCopyCellCarrier->SetDecouplingId(sDecouplingId);
pSulCopyCellCarrier->SetDecouplingEnabled(bEnableDecoupling);
LeaveCriticalSection(&m_CriticalSectionDB);
May be the above code works, but is it too much to put between "Enter" and "Leave" critical section ?
Re: Critical section begin end() changing the auto value
@kaud: Is there any workaround to avoid the multithreading issue ? (just for the release ) and change it later properly ?
Re: Critical section begin end() changing the auto value
I changed the code :
Code:
EnterCriticalSection(&m_CriticalSectionDB);
const auto & sDecouplingId = ulDlDecoupcells.m_bIsMultiplePair ? "NR cell decoupled more than once" : cpTddCell->GetID();
const auto& bEnableDecoupling = true;
pTddCopyCellCarrier->SetDecouplingId(sDecouplingId);
pTddCopyCellCarrier->SetDecouplingEnabled(bEnableDecoupling);
if (!BusinessObjects::BO_DBHelper::instance().Apply(pTddCopyCell.get(), pTddCopyCell.get()))
{
AI_Message::instance()->Add("Database Error. Failed to update cell: %s", pTddCopyCell->GetID());
LeaveCriticalSection(&m_CriticalSectionDB);
return;
}
pSulCopyCellCarrier->SetDecouplingId(sDecouplingId);
pSulCopyCellCarrier->SetDecouplingEnabled(bEnableDecoupling);
if (!BusinessObjects::BO_DBHelper::instance().Apply(pSulCopyCell.get(), pSulCopyCell.get()))
{
AI_Message::instance()->Add("Database Error. Failed to update cell: %s", pSulCopyCell->GetID());
LeaveCriticalSection(&m_CriticalSectionDB);
return;
}
LeaveCriticalSection(&m_CriticalSectionDB);
But still getting garbage value !!!!
Re: Critical section begin end() changing the auto value
That change would only be applicable if all changes to cpTddCell() were protected with a critical section.
What is the type of cpTddCell->GetID()? Why is sDecouplingId a ref - for performance reasons to avoid a copy?
Re: Critical section begin end() changing the auto value
Thankyou very much for looking into this and comments.
- GetID() is of type : const char* GetID() const override;
- sDecouplingId is made reference, just for performance reasons (so can be changed)
Re: Critical section begin end() changing the auto value
Do yourself a favor and use the RAII (Resource Acquisition Is Initialization) pattern rather than the explicit enter/leave critical section calls.
Why? Because code like this is problematic:
Code:
EnterCriticalSection(&m_CriticalSectionDB);
const auto & sDecouplingId = ulDlDecoupcells.m_bIsMultiplePair ? "NR cell decoupled more than once" : cpTddCell->GetID();
const auto& bEnableDecoupling = true;
pTddCopyCellCarrier->SetDecouplingId(sDecouplingId);
pTddCopyCellCarrier->SetDecouplingEnabled(bEnableDecoupling);
if (!BusinessObjects::BO_DBHelper::instance().Apply(pTddCopyCell.get(), pTddCopyCell.get()))
{
AI_Message::instance()->Add("Database Error. Failed to update cell: %s", pTddCopyCell->GetID());
LeaveCriticalSection(&m_CriticalSectionDB);
return;
}
pSulCopyCellCarrier->SetDecouplingId(sDecouplingId);
pSulCopyCellCarrier->SetDecouplingEnabled(bEnableDecoupling);
if (!BusinessObjects::BO_DBHelper::instance().Apply(pSulCopyCell.get(), pSulCopyCell.get()))
{
AI_Message::instance()->Add("Database Error. Failed to update cell: %s",
pSulCopyCell->GetID());
LeaveCriticalSection(&m_CriticalSectionDB);
return;
}
LeaveCriticalSection(&m_CriticalSectionDB);
The code is problematic because if ANY code after the call to EnterCriticalSection(...) throws an exception, the cs will never get released.
Instead use raii critical section wrapper classes so the cs is released when the class goes out of scope (regardless of whether an exception was thrown).
Re: Critical section begin end() changing the auto value
Quote:
Originally Posted by
pdk5
Thankyou very much for looking into this and comments.
- GetID() is of type : const char* GetID() const override;
- sDecouplingId is made reference, just for performance reasons (so can be changed)
You need to make sure that the char* pointer is valid throughout the execution of that section of code and isn't being changed (or contents overwritten) somewhere on a different thread.
In post #1 you stated:
Quote:
i found a shortcut and found and did the following change to it work
Code:
pSulCopyCellCarrier->SetDecouplingId(pTddCopyCellCarrier->GetDecouplingId());
pSulCopyCellCarrier->SetDecouplingEnabled(bEnableDecoupling);
Does:
Code:
pSulCopyCellCarrier->SetDecouplingId(cpTddCell->GetID());
pSulCopyCellCarrier->SetDecouplingEnabled(bEnableDecoupling);
work?
Re: Critical section begin end() changing the auto value
thanks a lot kaud.
Yes, the code
Code:
pSulCopyCellCarrier->SetDecouplingId(cpTddCell->GetID());
pSulCopyCellCarrier->SetDecouplingEnabled(bEnableDecoupling);
works !
Re: Critical section begin end() changing the auto value
Then the original issue is a threading one as it looks like that cpTddCell pointer is changed somewhere so that the ref to cpTddCell->GetID() becomes invalid and hence has 'garbage' data.
Re: Critical section begin end() changing the auto value
Thanks a lot kaud.
hope I can keep the following code for the release : (Assuming there are no issues with that !)
pSulCopyCellCarrier->SetDecouplingId(pTddCopyCellCarrier->GetDecouplingId());
pSulCopyCellCarrier->SetDecouplingEnabled(bEnableDecoupling);
I will keep investigating further
Re: Critical section begin end() changing the auto value
@Arjay: Thankyou for the comments, I will look into that. As of now i followed the pattern in the legacy code. Hopefully i can try improving it in the upcoming release.
Also it will be very helpful, if you can give me some kind of pseudocode in this case for me to check further ?
Re: Critical section begin end() changing the auto value
Quote:
Originally Posted by
pdk5
@Arjay: Thankyou for the comments, I will look into that. As of now i followed the pattern in the legacy code. Hopefully i can try improving it in the upcoming release.
Also it will be very helpful, if you can give me some kind of pseudocode in this case for me to check further ?
Click on the My Code Guru Articles link in my signature line. On page 2, you'll find the following two articles:
Win32 Thread Synchronization, Part I: Overview
Win32 Thread Synchronization, Part II: Helper Classes
Both articles contain wrapper classes for using RAII wrt thread synchronization.
For your code, using RAII (and the article's helper classes) the Enter/Leave critical section calls would be replaced with one call to CAutoLockT<>:
Code:
CAutoLockT< CLockableCS > lock( &m_csLock );
const auto & sDecouplingId = ulDlDecoupcells.m_bIsMultiplePair ? "NR cell decoupled more than once" : cpTddCell->GetID();
const auto& bEnableDecoupling = true;
pTddCopyCellCarrier->SetDecouplingId(sDecouplingId);
pTddCopyCellCarrier->SetDecouplingEnabled(bEnableDecoupling);
if (!BusinessObjects::BO_DBHelper::instance().Apply(pTddCopyCell.get(), pTddCopyCell.get()))
{
AI_Message::instance()->Add("Database Error. Failed to update cell: %s", pTddCopyCell->GetID());
return;
}
pSulCopyCellCarrier->SetDecouplingId(sDecouplingId);
pSulCopyCellCarrier->SetDecouplingEnabled(bEnableDecoupling);
if (!BusinessObjects::BO_DBHelper::instance().Apply(pSulCopyCell.get(), pSulCopyCell.get()))
{
AI_Message::instance()->Add("Database Error. Failed to update cell: %s",
pSulCopyCell->GetID());
return;
}
Re: Critical section begin end() changing the auto value