New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove a vptr race condition #4386

Merged
merged 2 commits into from Aug 24, 2017

Conversation

Projects
None yet
4 participants
@pirapira
Member

pirapira commented Aug 23, 2017

There was a race condition involving a worker thread that looks up vptrs.
The worker thread was joined by the base class's destructor, not the derived class's destructor.
So the worker thread kept working, looking up vptrs, even after the derived object was destroyed.

After fixing this problem, I saw I can remove one sleep(1 second) in BlockchainTest execution.

Thread sanitizer helped a lot.

With this command

ETHEREUM_TEST_PATH='../../tests' test/testeth -t 'BlockchainTests' -- --filltests --singletest  randomStatetest319BC

I saw

WARNING: ThreadSanitizer: data race on vptr (ctor/dtor vs virtual call) (pid=8000)                          
  Write of size 8 at 0x7b4c00002e78 by main thread:                                                                                                                                                                                                 
    #0 ~Worker /home/yh/src/cpp-ethereum/test/../libdevcore/Worker.h:64 (testeth+0x820eef)                                                                                                                                                                 
    #1 ~EthashCPUMiner /home/yh/src/cpp-ethereum/libethashseal/EthashCPUMiner.cpp:44 (testeth+0x9064ed)                                                                                                                                                    
    #2 ~EthashCPUMiner /home/yh/src/cpp-ethereum/libethashseal/EthashCPUMiner.cpp:43 (testeth+0x906559)                                                                                                                                       
    #3 std::_Sp_counted_ptr<dev::eth::GenericMiner<dev::eth::EthashProofOfWork>*, (__gnu_cxx::_Lock_policy)2>::_M_dispose() /usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/shared_ptr_base.h:372 (discriminator 1) (testeth+
0x8f95e0)                                    
    #4 std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() /usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/shared_ptr_base.h:150 (testeth+0x7b62d8)                                                              
    #5 ~__shared_count /usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/shared_ptr_base.h:662 (testeth+0x7b629e)                                                                                                              
    #6 ~__shared_ptr /usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/shared_ptr_base.h:928 (testeth+0x8dd25d)                                                                                                                
    #7 void std::_Destroy<std::shared_ptr<dev::eth::GenericMiner<dev::eth::EthashProofOfWork> > >(std::shared_ptr<dev::eth::GenericMiner<dev::eth::EthashProofOfWork> >*) /usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/stl
_construct.h:93 (testeth+0x8dd1f9)                                                                                                                                                                                                                         
    #8 void std::_Destroy_aux<false>::__destroy<std::shared_ptr<dev::eth::GenericMiner<dev::eth::EthashProofOfWork> >*>(std::shared_ptr<dev::eth::GenericMiner<dev::eth::EthashProofOfWork> >*, std::shared_ptr<dev::eth::GenericMiner<dev::eth::EthashProo
fOfWork> >*) /usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/stl_construct.h:103 (discriminator 1) (testeth+0x8dd1bf)                                    
    #9 void std::_Destroy<std::shared_ptr<dev::eth::GenericMiner<dev::eth::EthashProofOfWork> >*>(std::shared_ptr<dev::eth::GenericMiner<dev::eth::EthashProofOfWork> >*, std::shared_ptr<dev::eth::GenericMiner<dev::eth::EthashProofOfWork> >*) /usr/bin/
../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/bits/stl_construct.h:126 (testeth+0x8dd170)

  Previous read of size 8 at 0x7b4c00002e78 by thread T34:                                                                                                                
    #0 operator() /home/yh/src/cpp-ethereum/libdevcore/Worker.cpp:60 (testeth+0xb75567)                                                                                                                                                                
    #1 void std::_Bind_simple<dev::Worker::startWorking()::$_0 ()>::_M_invoke<>(std::_Index_tuple<>) /usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/functional:1390 (discriminator 2) (testeth+0xb753d9)
    #2 std::_Bind_simple<dev::Worker::startWorking()::$_0 ()>::operator()() /usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/functional:1380 (testeth+0xb75389)
    #3 std::thread::_State_impl<std::_Bind_simple<dev::Worker::startWorking()::$_0 ()> >::_M_run() /usr/bin/../lib/gcc/x86_64-linux-gnu/6.3.0/../../../../include/c++/6.3.0/thread:197 (testeth+0xb7524d)                                                  
    #4 std::error_code::default_error_condition() const ??:? (libstdc++.so.6+0xbb83e)
Remove a vptr race condition
There was a race condition involving a worker thread that looks up vptrs.
The worker thread was joined by the base class's destructor, not the derived class's destructor.
So the worker thread kept working, looking up vptrs, even after the derived class was destroyed.

After fixing this problem, I saw I can remove one sleep(1 second) in BlockchainTest execution.

Thread sanitizer helped a lot.

@pirapira pirapira requested review from chfast and winsvega Aug 23, 2017

@@ -412,7 +412,6 @@ json_spirit::mObject fillBCTest(json_spirit::mObject const& _input)
}
blArray.push_back(blObj); //json data
this_thread::sleep_for(chrono::seconds(1));

This comment has been minimized.

@winsvega

winsvega Aug 23, 2017

Contributor

I wonder why this line was here in the first place. have you tried to fill tests after this update?

@winsvega

winsvega Aug 23, 2017

Contributor

I wonder why this line was here in the first place. have you tried to fill tests after this update?

This comment has been minimized.

@pirapira

pirapira Aug 23, 2017

Member

Yes, I did. I saw no problems.

@pirapira

pirapira Aug 23, 2017

Member

Yes, I did. I saw no problems.

This comment has been minimized.

@pirapira

pirapira Aug 23, 2017

Member

Thread sanitizer reported that the vptr read and write are separated by this sleep.

@pirapira

pirapira Aug 23, 2017

Member

Thread sanitizer reported that the vptr read and write are separated by this sleep.

@winsvega

maybe that was the reason of some bugs?

@chfast

chfast approved these changes Aug 23, 2017

Not very nice, but let it be.

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Aug 24, 2017

Member

Appveyor timeouts.

Member

pirapira commented Aug 24, 2017

Appveyor timeouts.

@pirapira pirapira merged commit 3ea9c38 into develop Aug 24, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chriseth chriseth removed the in progress label Aug 24, 2017

@pirapira pirapira deleted the remove-vptr-race branch Aug 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment