-
August 12th, 2020, 10:21 AM
#1
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
Last edited by 2kaud; August 13th, 2020 at 02:19 AM.
-
August 12th, 2020, 10:52 AM
#2
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.
All advice is offered in good faith only. All my code is tested (unless stated explicitly otherwise) with the latest version of Microsoft Visual Studio (using the supported features of the latest standard) and is offered as examples only - not as production quality. I cannot offer advice regarding any other c/c++ compiler/IDE or incompatibilities with VS. You are ultimately responsible for the effects of your programs and the integrity of the machines they run on. Anything I post, code snippets, advice, etc is licensed as Public Domain https://creativecommons.org/publicdomain/zero/1.0/ and can be used without reference or acknowledgement. Also note that I only provide advice and guidance via the forums - and not via private messages!
C++23 Compiler: Microsoft VS2022 (17.6.5)
-
August 12th, 2020, 11:00 AM
#3
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 ?
Last edited by 2kaud; August 13th, 2020 at 02:21 AM.
-
August 12th, 2020, 11:14 AM
#4
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 ?
-
August 12th, 2020, 11:41 AM
#5
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 !!!!
Last edited by 2kaud; August 13th, 2020 at 02:22 AM.
-
August 12th, 2020, 11:54 AM
#6
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?
All advice is offered in good faith only. All my code is tested (unless stated explicitly otherwise) with the latest version of Microsoft Visual Studio (using the supported features of the latest standard) and is offered as examples only - not as production quality. I cannot offer advice regarding any other c/c++ compiler/IDE or incompatibilities with VS. You are ultimately responsible for the effects of your programs and the integrity of the machines they run on. Anything I post, code snippets, advice, etc is licensed as Public Domain https://creativecommons.org/publicdomain/zero/1.0/ and can be used without reference or acknowledgement. Also note that I only provide advice and guidance via the forums - and not via private messages!
C++23 Compiler: Microsoft VS2022 (17.6.5)
-
August 12th, 2020, 12:41 PM
#7
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)
Last edited by pdk5; August 12th, 2020 at 12:45 PM.
-
August 12th, 2020, 07:05 PM
#8
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).
Last edited by 2kaud; August 13th, 2020 at 02:17 AM.
-
August 13th, 2020, 02:15 AM
#9
Re: Critical section begin end() changing the auto value
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:
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?
Last edited by 2kaud; August 13th, 2020 at 02:49 AM.
All advice is offered in good faith only. All my code is tested (unless stated explicitly otherwise) with the latest version of Microsoft Visual Studio (using the supported features of the latest standard) and is offered as examples only - not as production quality. I cannot offer advice regarding any other c/c++ compiler/IDE or incompatibilities with VS. You are ultimately responsible for the effects of your programs and the integrity of the machines they run on. Anything I post, code snippets, advice, etc is licensed as Public Domain https://creativecommons.org/publicdomain/zero/1.0/ and can be used without reference or acknowledgement. Also note that I only provide advice and guidance via the forums - and not via private messages!
C++23 Compiler: Microsoft VS2022 (17.6.5)
-
August 13th, 2020, 03:44 AM
#10
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 !
-
August 13th, 2020, 04:01 AM
#11
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.
All advice is offered in good faith only. All my code is tested (unless stated explicitly otherwise) with the latest version of Microsoft Visual Studio (using the supported features of the latest standard) and is offered as examples only - not as production quality. I cannot offer advice regarding any other c/c++ compiler/IDE or incompatibilities with VS. You are ultimately responsible for the effects of your programs and the integrity of the machines they run on. Anything I post, code snippets, advice, etc is licensed as Public Domain https://creativecommons.org/publicdomain/zero/1.0/ and can be used without reference or acknowledgement. Also note that I only provide advice and guidance via the forums - and not via private messages!
C++23 Compiler: Microsoft VS2022 (17.6.5)
-
August 13th, 2020, 04:18 AM
#12
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
-
August 13th, 2020, 04:19 AM
#13
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 ?
Last edited by pdk5; August 13th, 2020 at 04:58 AM.
-
August 13th, 2020, 05:57 AM
#14
Re: Critical section begin end() changing the auto value
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;
}
-
August 13th, 2020, 07:58 AM
#15
Re: Critical section begin end() changing the auto value
Tags for this Thread
Posting Permissions
- You may not post new threads
- You may not post replies
- You may not post attachments
- You may not edit your posts
-
Forum Rules
|
Click Here to Expand Forum to Full Width
|