Skip to content
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

Set PThread::m_isRunning early, when the thread is created #16

Merged

Conversation

Lastique
Copy link
Contributor

This allows PThread::IsTerminated to return false as soon as the thread is created, which may be earlier than PThread::PX_ThreadStart manages to set it. This should fix issues when the code synchronously tests if the thread is terminated soon after spawning it.

This may fix issues reported in comments to #11.

This allows PThread::IsTerminated to return false as soon as the thread
is created, which may be earlier than PThread::PX_ThreadStart manages
to set it. This should fix issues when the code synchronously tests
if the thread is terminated soon after spawning it.
@willamowius
Copy link
Owner

willamowius commented Feb 21, 2022

When testing with callgen323 the crash during runtime is gone for me with this patch and Valgrind doesn't complain either.

I do get a crash in the exit handler though and one of the registration threads seems to hang a few seconds.

(gdb) bt
#0 __memcmp_avx2_movbe () at ../sysdeps/x86_64/multiarch/memcmp-avx2-movbe.S:412
#1 0x00005648abc6e38a in std::char_traits::compare (__s1=0x5648ad2b3de0 "h323PluginCodecManager",
__s2=0x564800000000 <error: Cannot access memory at address 0x564800000000>, __n=22) at /usr/include/c++/11/bits/char_traits.h:361
#2 0x00005648abc7848a in std::__cxx11::basic_string<char, std::char_traits, std::allocator >::compare (this=0x5648ad2dbfc0, __str="")
at /usr/include/c++/11/bits/basic_string.h:2878
#3 0x00005648abc774c6 in std::operator<=><char, std::char_traits, std::allocator >(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const&, std::__cxx11::basic_string<char, std::char_traits, std::allocator > const&) (__lhs="h323PluginCodecManager",
__rhs="") at /usr/include/c++/11/bits/basic_string.h:6263
#4 0x00005648abc7688d in std::less<std::__cxx11::basic_string<char, std::char_traits, std::allocator > >::operator() (this=0x5648ad2dbeb8,
__x="h323PluginCodecManager", __y="") at /usr/include/c++/11/bits/stl_function.h:386
#5 0x00005648abc7b270 in std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits, std::allocator >, std::pair<std::__cxx11::basic_string<char, std::char_traits, std::allocator > const, PFactory<PPluginModuleManager, std::__cxx11::basic_string<char, std::char_traits, std::allocator > >::WorkerBase*>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits, std::allocator > const, PFactory<PPluginModuleManager, std::__cxx11::basic_string<char, std::char_traits, std::allocator > >::WorkerBase*> >, std::less<std::__cxx11::basic_string<char, std::char_traits, std::allocator > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits, std::allocator > const, PFactory<PPluginModuleManager, std::__cxx11::basic_string<char, std::char_traits, std::allocator > >::WorkerBase*> > >::equal_range (this=0x5648ad2dbeb8, __k="") at /usr/include/c++/11/bits/stl_tree.h:1971
#6 0x00005648abc7b17c in std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits, std::allocator >, std::pair<std::__cxx11::basic_string<char, std::char_traits, std::allocator > const, PFactory<PPluginModuleManager, std::__cxx11::basic_string<char, std::char_traits, std::allocator > >::WorkerBase*>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits, std::allocator > const, PFactory<PPluginModuleManager, std::__cxx11::basic_string<char, std::char_traits, std::allocator > >::WorkerBase*> >, std::less<std::__cxx11::basic_string<char, std::char_traits, std::allocator > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits, std::allocator > const, PFactory<PPluginModuleManager, std::__cxx11::basic_string<char, std::char_traits, std::allocator > >::WorkerBase*> > >::erase (this=0x5648ad2dbeb8, __x="") at /usr/include/c++/11/bits/stl_tree.h:2508
#7 0x00005648abc7b143 in std::map<std::cxx11::basic_string<char, std::char_traits, std::allocator >, PFactory<PPluginModuleManager, std::--Type for more, q to quit, c to continue without paging--
cxx11::basic_string<char, std::char_traits, std::allocator > >::WorkerBase*, std::less<std::__cxx11::basic_string<char, std::char_traits, std::allocator > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits, std::allocator > const, PFactory<PPluginModuleManager, std::__cxx11::basic_string<char, std::char_traits, std::allocator > >::WorkerBase*> > >::erase (this=0x5648ad2dbeb8, __x="")
at /usr/include/c++/11/bits/stl_map.h:1069
#8 0x00005648abc7b109 in PFactory<PPluginModuleManager, std::__cxx11::basic_string<char, std::char_traits, std::allocator > >::Unregister_Internal (this=0x5648ad2dbe80, key="") at /root/h323plus/../ptlib/include/ptlib/pfactory.h:367
#9 0x00005648abc7afda in PFactory<PPluginModuleManager, std::__cxx11::basic_string<char, std::char_traits, std::allocator > >::Unregister (
key="") at /root/h323plus/../ptlib/include/ptlib/pfactory.h:266
#10 0x00005648abc7a912 in PFactory<PPluginModuleManager, std::__cxx11::basic_string<char, std::char_traits, std::allocator > >::Worker::~Worker (this=0x5648ac2dfdc0 , __in_chrg=)
at /root/h323plus/../ptlib/include/ptlib/pfactory.h:226
#11 0x00007fe5a4e5f4e5 in __run_exit_handlers (status=0, listp=0x7fe5a502e818 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true,
run_dtors=run_dtors@entry=true) at exit.c:113
#12 0x00007fe5a4e5f660 in __GI_exit (status=) at exit.c:143
#13 0x00007fe5a4e43fd7 in __libc_start_call_main (main=main@entry=0x5648abc64f9a <main(int, char**, char**)>, argc=argc@entry=28,
argv=argv@entry=0x7ffee7561618) at ../sysdeps/nptl/libc_start_call_main.h:74
#14 0x00007fe5a4e4407d in __libc_start_main_impl (main=0x5648abc64f9a <main(int, char**, char**)>, argc=28, argv=0x7ffee7561618, init=,
fini=, rtld_fini=, stack_end=0x7ffee7561608) at ../csu/libc-start.c:409
#15 0x00005648abc64ed5 in _start ()

@Lastique Lastique deleted the fix_pthread_late_set_is_running branch February 21, 2022 17:59
@Lastique
Copy link
Contributor Author

This seems like a global initialization/termination order issue.

@folarte
Copy link

folarte commented Feb 22, 2022

My tests are good too with this patch. After sleeping on it I see moving the setting of m_isrunning also moves it from the new thread to the starter thread, which is not just a quantitative (a bit earlier), but a qualitative change, as it closes the race condition where the starter can see the new thread as terminated after the PThread start methods returns but before it runs.

Also h323+ was not stopping properly in my code, the daemon hung forever or was killed by systemctl, now it terminates properly. It was caused by the same pthread_kill problem and is fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants