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

use sigwait for thread-safe and multi-thread-aware signal handling #12904

Merged
merged 8 commits into from
Nov 24, 2015

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Sep 1, 2015

this is a redesign of the unix signal handling so that it will be more thread-safe and extensible.

yyc: Close #14091

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Oct 30, 2015

bump. rebased now that jn/threading is merged.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Nov 23, 2015

this branch should be ready-to-merge content-wise (aside from needing rebase again it seems). I couldn't reproduce the x86 failure locally. Would be great if someone could review and see if anything looks amiss.

extern ptrint_t bt_data[MAX_BT_SIZE+1];
extern size_t bt_size;
extern JL_THREAD ptrint_t bt_data[MAX_BT_SIZE+1];
extern JL_THREAD size_t bt_size;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed JL_THREAD in #14083 since it's not used anymore. Should I add it back or should these go into the tls struct as well?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these can go in the tls struct (or only one thread could throw/handle an exception at a time)

}
jl_throw_in_thread(0, thread, excpt);
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to pass the real tid here

@yuyichao
Copy link
Contributor

I've rebased this branch on top of the current master (#14083) at yyc/flex_signals-rebase and also fixed some compiler warnings there.

I think we can probably also clean up jl_thread_task_state_t a little since we can now simply store a pointer to jl_tls_states in it.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Nov 23, 2015

Sounds good. Do you want to go ahead and force-push your branch here and merge this when you are happy with it (possibly squashing the commits a little)?

@yuyichao
Copy link
Contributor

I can do that. What about the TODO you have above?

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Nov 23, 2015

there's other (preexisting) bug there, so it doesn't need to be a blocking item. just don't cause segfaults (aka stackoverflow) on child threads for now.

@yuyichao
Copy link
Contributor

OK. Done. The code should be identical with the version before apart from some clean up in the jl_thread_task_state_t as mentioned above.

I didn't squash too much since I'm not really familiar with signal handling. I only merge my compiler warning fixes into each individual commits and merge the compilation fix in Oscar's commit to the previous one so that this PR is bisect-able on linux without too many compiler warnings.

@yuyichao
Copy link
Contributor

I think I'm happy with it apart from the intermittent AppVeyor win64 freeze that happend twice on this commit.... I'd still like someone to check my rebase but I think I'll merge it tomorrow otherwise.

@tkelman Have you checked if Oscar's fix in this PR solves the race for you on windows without triggering a libuv assertion when threading enabled?

@tkelman
Copy link
Contributor

tkelman commented Nov 24, 2015

On your rebase version of this branch (more or less the same right?) everything looked to be in order with llvm 3.3 and threading enabled, so 👍 from me. We could maybe think about turning threading on by default already.

@yuyichao
Copy link
Contributor

Cool.

more or less the same right

Yes, apart from some clean up that I only pushed to this branch.

yuyichao added a commit that referenced this pull request Nov 24, 2015
use sigwait for thread-safe and multi-thread-aware signal handling
@yuyichao yuyichao merged commit 6472a57 into master Nov 24, 2015
@yuyichao yuyichao deleted the jn/flex_signals branch November 24, 2015 13:40
@tkelman
Copy link
Contributor

tkelman commented Nov 24, 2015

I will have to check whether this PR made any difference here but it's worth noting that the MSVC build with LLVM 3.3 and threading enabled freezes at the start of bootstrap.

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.

4 participants