Re: Instability with signals and threads

[Mikulas Patocka <mikulas@artax.karlin.mff.cuni.cz>]

> Never mind that.  I can fix your testcase by calling _my_tls.remove with 
> INFINITE as parameter in both places.  If I drop one of them, your 
> testcase will invariable fail at one point.  With both INFINITE params 
> in place, your testcase is now running half an hour without problems.

For me, this change doesn't fix the testcase, it just reduces the 
probability that it hangs.

With this change, the testcase still locks up, but with a different 
stacktrace:
thread1:
        Sleep
        _yield
        pthread::create
        sigdelayed ??
        _cygwin_exit_return ??
        _cygtls::call2

thread2:
        SetEvent
        muto::release
        init_cygheap::find_tls
        _cygtls::init_thread

thread3:
        WriteFile
        sig_send
        timer_thread
        cygthread::callfunc
        cygthread::stub
        _cygtls::call2

thread4:
        VirtualFree 
        thread_wrapper

thread5:
        only ntdll stuff

So, apparently, there is another bug, where thread->cygtls isn't being set 
and pthread::create loops endlessly calling yield.

> Thinking about it, the fact that _cygtls::remove allows to apply a 
> non-INFINITE wait is rather strange, isn't it?  Calling remove_tls with 
> a 0 wait, it allows to return the function silently, without actually 
> having removed the thread from the list.  This is bound to go downhill 
> at one point and looks like a kludge to me to circumvent some potential 
> hang in another situation...

Looking at CVS history, the "wait" argument was added to cygtls.cc version 
1.2 with a comment: "Add a 'wait' argument to control how long we wait for 
a lock before removing." There is no explanation why is it needed.

> I'm not exactly sure if that works as intended.  I will apply this patch 
> and create a new Cygwin snapshot on https://cygwin.com/snapshots/ in a 
> couple of minutes.  I'd appreciate if you and others would give it an 
> exhaustive test.  New spurious hangs or SEGVs in other situations which 
> so far worked fine would be good indicators for another problem in the 
> code.

Yes, I think it's correct to remove the wait argument.

> Other than that, there's certainly some room for improvement.  Calling 
> threadlist[idx]->remove from the find_tls exception handler looks 
> extremly hairy to me.  I wonder if that should be called at all at this 
> point, or if there shouldn't be better some "simplified" removal 
> operation which doesn't require the _cygtls pointer.  If the thread 
> doesn't exist anymore, so does its _cygtls area.

I suggest to remove that exception handler at all. This thing can't ever 
work reliably - it could reduce probability of crashes but not eliminate 
them. Even if we handled the page fault correctly - what happens if some 
other thread allocates a different object at the location that belonged to 
the tls before? - then find_tls thinks that this different object is tls 
and corrupts it.

I suggest to remove the exception handler and if it results in any 
regressions, fix them properly.

Mikulas

> Thanks, Corinna -- 
> Corinna Vinschen

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple