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.

Sign up for GitHub

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

Jump to bottom

gscan: handle ctrl-c #2257

Merged
merged 2 commits into from
May 3, 2017
Merged

gscan: handle ctrl-c #2257

merged 2 commits into from
May 3, 2017

Conversation

matthewrmshin
Copy link
Contributor

Fix #2195.

@matthewrmshin matthewrmshin added this to the soon milestone Apr 27, 2017
@matthewrmshin matthewrmshin self-assigned this Apr 27, 2017
@matthewrmshin
Copy link
Contributor Author

Not ready for review yet.

@matthewrmshin matthewrmshin modified the milestones: next release, soon May 2, 2017
oliver-sanders
oliver-sanders reviewed May 2, 2017
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

This seems to be working, both cylc scan and cylc gscan seem to be exiting cleanly killing all processes.

I am seeing some traceback when killing cylc scan and some cylc scan output when killing cylc gscan which ideally we would suppress.

@matthewrmshin
Copy link
Contributor Author

You should no longer see traceback.

@oliver-sanders
Copy link
Member

Looks good. I can still get traceback (or worse) from terminating either scan or gscan within the first second of execution, this probably isn't worth worrying about though.

@matthewrmshin
Copy link
Contributor Author

I am happy with some traceback, but not happy with worse (e.g. zombie processes, hang up, etc.)

oliver-sanders
oliver-sanders approved these changes May 3, 2017
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

It is still possible to get traceback etc by:

  • Interrupting the process immediately after executing it (affected logic is in bin/cylc so is outside the scope of this pull).
  • Interrupting the process then interrupting it again (the new 'stopping' message should help to prevent this).

I think this is acceptable.

dvalters
dvalters approved these changes May 3, 2017
View reviewed changes
Copy link
Contributor

@dvalters dvalters left a comment

Choose a reason for hiding this comment

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

Works well.

@dvalters dvalters merged commit 4515666 into cylc:master May 3, 2017
@matthewrmshin matthewrmshin deleted the fix-gscan-ctrl-c branch May 3, 2017 16:44
matthewrmshin added a commit to matthewrmshin/cylc-flow that referenced this pull request May 5, 2017
@matthewrmshin
Fix gpanel broken by cylc#2257
e510329
@matthewrmshin matthewrmshin mentioned this pull request May 5, 2017
Merged
oliver-sanders added a commit that referenced this pull request May 5, 2017
@oliver-sanders
Merge pull request #2264 from matthewrmshin/fix-gpanel
26e6b91 
Fix gpanel broken by #2257
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvalters dvalters dvalters approved these changes

@oliver-sanders oliver-sanders oliver-sanders approved these changes

Assignees

@matthewrmshin matthewrmshin

Labels
None yet
Projects
None yet
Milestone
cylc-7.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@matthewrmshin @oliver-sanders @dvalters