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

Explicit system signal handlers #776

Merged
merged 105 commits into from Dec 6, 2016
Merged

Explicit system signal handlers #776

merged 105 commits into from Dec 6, 2016

Conversation

Lestropie
Copy link
Member

Awaiting confirmation from the supreme allied commander @jdtournier that we're going to go with this solution, and testing on Mac.

Unfortunately the handling doesn't work on my Windows laptop; hence testing the capability in configure and only enabling the feature if an environment variable is explicitly set (so compilation doesn't fail on such systems if the update is pulled but configure is not re-run).

Closes #758.

dchristiaens and others added 8 commits May 26, 2016 10:42
Threading issue likely originated from maintaining data structure integrity for random particle selection. Now removed list of pointers and shifted this functionality to ParticlePool, to avoid any accidental pointer invalidation.
Test for ability to set signal handlers at configure step, and generate static lookup table of error messages during configure step.
Delete piped images if program is terminated abruptly.
@jdtournier
Copy link
Member

jdtournier commented Sep 6, 2016 via email

@Lestropie
Copy link
Member Author

Can you explain what's not working on Windows?

It claims that some of the structures / functions used in constructing the handlers are not defined, despite having found the appropriate system header file. But there has been other weirdness associated with my Windows install so who knows if it's just me.

And why the need to re-configure?

If certain systems (like mine) don't provide adequate support for constructing the signal handlers, then pulling such an update would cause compilation to fail, which would result in a lot of angry discussion forum posts. This is avoided by testing for support of the functionality in configure, and (if successful) adding a new environment variable MRTRIX_SIGNAL_HANDLING to the config file that triggers compilation of the relevant code. So if you git pull but don't re-run configure, this variable won't be defined, the relevant code won't be included in compilation, and the default handlers will remain in place.

@jdtournier
Copy link
Member

OK, sure, it it fails to compile on certain platforms, you'd need to deal with that. But wouldn't it be possible to prevent the use of signal handlers on Windows while we figure out how to get it working, and assume it's always supported on Linux and MacOSX? In which case all you'd need is the already-defined MRTRIX_WINDOWS macro...?

@Lestropie
Copy link
Member Author

Possibly. But I don't know whether or not it works on different versions of Windows (I already know that the same MSYS installer file results in different installations on Windows 7 and Windows 8.1), it hasn't been tested on Mac, so I went for the direct testing approach without assumptions. If you'd rather do more widespread testing in the hope of not depending on a configure re-run we can do that.

@jdtournier
Copy link
Member

Yes, it would be worth checking on MacOSX - I don't expect any problems particularly, it's a UNIX system. Windows on the other hand will rely on some serious MinGW boilerplate code...

On Windows, use the semi-depreciated signal() function rather than sigaction(). This also means that testing signal handling as part of the configure script is no longer necessary.
Use basic C functions and an atomic_flag in signal handling to reduce chances of further corruption.
@Lestropie
Copy link
Member Author

Did some more tweaking on this since I want to include it in the upcoming tag. No longer requires re-running configure. The #ifdef usage may be rather liberal, but I'd rather avoid the risk of breaking compilation for someone because of a missing symbol.

Remaining question is to do with standalone installations. While the signal symbol names should be consistent between systems, the integer codes aren't guaranteed to be. Therefore, if compiling on one system and running on another, incorrect error messages may be displayed. I could either add an environment variable flag to configure to indicate to not use signal handlers, or just add a warning to the standalone installation docs that the warnings may be wrong (they do also provide the integer signal code).

@Lestropie Lestropie mentioned this pull request Sep 23, 2016
dchristiaens and others added 18 commits November 18, 2016 17:19
Fix multithreading issues in global tractography.
mrview: display extended gbtable in properties box
This addresses the huge drop in performance observed with Eigen 3.3, as
discussed in #833.
dwidenoise: add brackets to final recombine step
mrtransform: apply the inverse transformation to DW gradients
@jdtournier
Copy link
Member

Just thought I'd update this thread with what I've been trying. The hurdle right now is getting this to work on Windows. It's not a deal-breaker since it doesn't interfere with current behaviour, and basically doesn't change anything - temporaries don't get cleaned just as is the case now... But it would be nice to get it to work.

So I've tried @Lestropie's SetConsoleCtrlHandler suggestion, but it still doesn't work... I've written a small example and posted that on the MSYS2 users mailing list, see whether there is a known solution for this.

Otherwise, we're still waiting on confirmation that this works on MacOSX... Anyone....? 😉

@Lestropie
Copy link
Member Author

Sounds like on Windows it's the terminal emulator gobbling up the signal, so not something we're likely to have a quick solution to. But the code might as well sit there just in case.

I might merge this into tag_0.3.16, and that way it'll get tested on Mac.

@Lestropie Lestropie changed the base branch from master to tag_0.3.16 December 6, 2016 01:24
@Lestropie Lestropie merged commit 6d8e9bd into tag_0.3.16 Dec 6, 2016
@jdtournier
Copy link
Member

Not sure it's simple as the terminal eating it up. Git manages to catch the interrupt (says Killed by signal 2. on the terminal). A quick google search shows this is how they handle things. A bit of a hack, but it seems to work... Worth investigating?

@Lestropie
Copy link
Member Author

I'd seen more abstract descriptions of that type of solution. But because we're not waiting on user input via fgetc() or similar, you have to pull some trick whereby you spawn a new thread to run your actual program, and have the primary process just monitoring keyboard inputs, with appropriate setup such that Ctrl-C is sent as keyboard input rather than a signal. Sounds to me like too much work for something that's a new and non-critical feature.

P.S. When you say "Git catches the interrupt", do you mean git, or the "Git for Bash" terminal? My link suggests that it's an issue that's common to a lot of terminal emulators built around Cygwin, but isn't necessarily guaranteed to be an issue for all of them - hence why nobody's taken responsibility and hunted down the source of the issue.

Lestropie added a commit that referenced this pull request Dec 7, 2016
First attempt at catching system signals from within Python scripts library, and generating appropriate message / performing appropriate cleanup.
Related to #758 and #776.
@jdtournier
Copy link
Member

OK, clearly you've had a good look through this, it's a lot more complicated than it looks... Let's shelve and let them sort this out in due course.

P.S. When you say "Git catches the interrupt", do you mean git, or the "Git for Bash" terminal?

That was git within the MSYS2 default terminal. Haven't checked the 'Git for Bash' (did you mean 'Git for Windows'?) one, but I'd expect it would behave just the same. My little test command did anyway, in that it didn't catch the interrupt in either terminals...

Anyway, let's call it a day for now...

@Lestropie
Copy link
Member Author

Ah. I think the download is called "Git for Windows", but the shortcut you get in the Start Menu gets called "Git Bash". Whatevs.

@jdtournier jdtournier deleted the glibc_handlers branch February 2, 2017 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants