CodeGuru Home VC++ / MFC / C++ .NET / C# Visual Basic VB Forums Developer.com
Page 1 of 2 12 LastLast
Results 1 to 15 of 17
  1. #1
    Join Date
    May 2015
    Posts
    500

    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.

  2. #2
    2kaud's Avatar
    2kaud is offline Super Moderator Power Poster
    Join Date
    Dec 2012
    Location
    England
    Posts
    7,822

    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)

  3. #3
    Join Date
    May 2015
    Posts
    500

    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.

  4. #4
    Join Date
    May 2015
    Posts
    500

    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 ?

  5. #5
    Join Date
    May 2015
    Posts
    500

    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.

  6. #6
    2kaud's Avatar
    2kaud is offline Super Moderator Power Poster
    Join Date
    Dec 2012
    Location
    England
    Posts
    7,822

    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)

  7. #7
    Join Date
    May 2015
    Posts
    500

    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.

  8. #8
    Arjay's Avatar
    Arjay is offline Moderator / EX MS MVP Power Poster
    Join Date
    Aug 2004
    Posts
    13,490

    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.

  9. #9
    2kaud's Avatar
    2kaud is offline Super Moderator Power Poster
    Join Date
    Dec 2012
    Location
    England
    Posts
    7,822

    Re: Critical section begin end() changing the auto value

    Quote Originally Posted by pdk5 View Post
    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)

  10. #10
    Join Date
    May 2015
    Posts
    500

    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 !

  11. #11
    2kaud's Avatar
    2kaud is offline Super Moderator Power Poster
    Join Date
    Dec 2012
    Location
    England
    Posts
    7,822

    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)

  12. #12
    Join Date
    May 2015
    Posts
    500

    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

  13. #13
    Join Date
    May 2015
    Posts
    500

    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.

  14. #14
    Arjay's Avatar
    Arjay is offline Moderator / EX MS MVP Power Poster
    Join Date
    Aug 2004
    Posts
    13,490

    Re: Critical section begin end() changing the auto value

    Quote Originally Posted by pdk5 View Post
    @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; 					
    }

  15. #15
    Join Date
    May 2015
    Posts
    500

    Re: Critical section begin end() changing the auto value

    Thanks a lot Arjay

Page 1 of 2 12 LastLast

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
  •  





Click Here to Expand Forum to Full Width

Featured