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

Update current test module details when cancelling running job #941

Merged
merged 1 commit into from
Apr 9, 2018

Conversation

Martchus
Copy link
Contributor

@Martchus Martchus commented Apr 5, 2018

@coveralls
Copy link

coveralls commented Apr 5, 2018

Coverage Status

Coverage decreased (-0.06%) to 57.159% when pulling c2532e2 on Martchus:upload_after_cancel into c39f386 on os-autoinst:master.

* Ensures the JSON file for a canceled test is created by handling
  signal TERM in autotest.pm.
* See https://progress.opensuse.org/issues/13524
* Only one half of the implementation. The other half is done
  in openQA.
sub handle_sigterm {
my ($sig) = @_;

if ($current_test) {
Copy link
Contributor

Choose a reason for hiding this comment

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

having so much code in a signal handler sounds like a bad idea.

Copy link
Contributor Author

@Martchus Martchus Apr 6, 2018

Choose a reason for hiding this comment

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

So the same rules as in C/C++ apply to Perl as well?

Then I should better just set the Perl-equivalent of a volatile variable here and check for it during test execution. But how would I do this check in practice? I mean this would require changing test code to handle the cancel. Or modifying the API functions to handle the cancel which sounds quite messy.

Copy link
Contributor Author

@Martchus Martchus Apr 6, 2018

Choose a reason for hiding this comment

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

So the same rules as in C/C++ apply to Perl as well?

If the answer to this question is yes we should definitely not merge this. Fortunately, Perl seems to take care of the usual problems when catching signal handlers:

At the C level, signals can interrupt just about anything. Unfortunately, this means that signals could interrupt Perl while Perl is changing its own internal data structures, leaving those data structures inconsistent and leading to a core dump. As of Perl 5.8, Perl tries very hard to ensure that this doesn't happen—when you install a signal handler, Perl installs a C-level signal handler that says "Perl received this signal." When Perl's data structures are consistent (after each operation it performs), the Perl interpreter checks to see whether a signal was received. If one was, your signal handler is called.

from https://docstore.mik.ua/orelly/perl4/cook/ch16_18.htm

When I understand this correctly, the handler is just like any other Perl function. It is invoked by the Perl runtime which ensures we're in a consistent state (and not got interrupted inside eg. malloc() which can happen in C/C++). So I would assume nothing bad can happen when executing arbitrary Perl code in the handler. I also just tested what happens when a die happens inside the handler and it worked as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with perl sighandlers aren't comparable to C sighandlers. That's true - and I don't think there is a problem with this PR. But beware the beginning I say :)

$current_test is mostly harmless, but if you e.g. access the $backend you never know in what it is. And the more code you have in signal handlers the more likely you run into such problems.

@coolo coolo merged commit 2588e57 into os-autoinst:master Apr 9, 2018
@Martchus Martchus deleted the upload_after_cancel branch April 9, 2018 13:09
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