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

cucumber trapping SIGINT behavior #988

Closed
akostadinov opened this issue Jun 13, 2016 · 26 comments · Fixed by #1353
Closed

cucumber trapping SIGINT behavior #988

akostadinov opened this issue Jun 13, 2016 · 26 comments · Fixed by #1353
Labels
🧷 pinned Tells Stalebot not to close this issue

Comments

@akostadinov
Copy link
Contributor

Hello, I observe the following behavior.
= regular cucumber vs SIGINT

  • test is running
  • hit ctrl+c
  • test continue to be running and program exits only after it completes fully
  • hit ctrl+c again
  • cucumber exits without any clean-up taking place

= cucumber with Signal.trap('SIGINT') { exit(false) }

  • test running
  • hit ctrl+c
  • test terminates and cucumber switches to After hook

I think that the latter behavior makes much more sense. If user wants to interrupt, then ideally that will happen as soon as possible while After hook is executed to clean-up any underlying test environment. This is current behavior for SIGTERM.

So I was wondering what was the idea behind default SIGINT trap behavior. Why not keep same as SIGTERM? Not a big issue for me as I have a workaround. But would be nice to make behavior more convenient OOB for other users.

btw very nice cucumber takes care to run After hook on SIGTERM. I made some effort to put clean-up in at_exit when After is skipped but it seems After is never skipped. Or was the idea to have single Ctrl+C terminate after scenario finish, second Ctrl+C to drop into After hook? In such case it is not working like that.

moved from cucumber/cucumber-ruby-core/issues/108

@danascheider
Copy link
Contributor

@mattwynne @brasmusson @tooky What do you think about this suggestion? Shouldn't be hard to implement but I'm not sure if it's what we want to do.

@stale
Copy link

stale bot commented Nov 8, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

@stale stale bot added the ⌛ stale Will soon be closed by stalebot unless there is activity label Nov 8, 2017
@akostadinov
Copy link
Contributor Author

bump

@stale stale bot removed the ⌛ stale Will soon be closed by stalebot unless there is activity label Nov 8, 2017
@stale
Copy link

stale bot commented Jan 7, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

@stale stale bot added the ⌛ stale Will soon be closed by stalebot unless there is activity label Jan 7, 2018
@akostadinov
Copy link
Contributor Author

bump, this needs project owners decision, not hard to implement

@stale stale bot removed the ⌛ stale Will soon be closed by stalebot unless there is activity label Jan 7, 2018
@stale
Copy link

stale bot commented Mar 8, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

@stale stale bot added the ⌛ stale Will soon be closed by stalebot unless there is activity label Mar 8, 2018
@akostadinov
Copy link
Contributor Author

bump

@stale stale bot removed the ⌛ stale Will soon be closed by stalebot unless there is activity label Mar 8, 2018
@xtrasimplicity
Copy link
Member

@olleolleolle, do you have any thoughts on this?

@olleolleolle
Copy link
Contributor

I came late to the codebase. I can’t offer archeological evidence for “why” things are the way they are.

That said, I like the clarity offered by having the effects of the current TERM signal at Ctrl-c in the example.

@xtrasimplicity
Copy link
Member

Thanks Olle. I agree, and can't really think of any real downsides to implementing this. If someone were to create a PR to change this (including any tests etc), do you think the core team would be open to merging it?

@olleolleolle
Copy link
Contributor

@xtrasimplicity I believe they would, but I wouldn't merge it myself, on account of not having "the full picture".

@xtrasimplicity
Copy link
Member

Yep, totally understandable. Thanks for clarifying. :)

@stale
Copy link

stale bot commented May 8, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

@stale stale bot added the ⌛ stale Will soon be closed by stalebot unless there is activity label May 8, 2018
@xtrasimplicity xtrasimplicity removed the ⌛ stale Will soon be closed by stalebot unless there is activity label May 8, 2018
@xtrasimplicity xtrasimplicity self-assigned this May 8, 2018
@xtrasimplicity xtrasimplicity added the 🧷 pinned Tells Stalebot not to close this issue label May 8, 2018
@xtrasimplicity xtrasimplicity removed their assignment Sep 29, 2018
@luke-hill
Copy link
Contributor

Just to add, it's not just hitting ctrl+c, it's other "kill and stop", for example on RubyMine or other IDE's when you want to stop the process.

@akostadinov - What's the issue with the current functionality, are you just wanting to add the "Run After hooks" bit. Because If you're wanting to kill something manually, in my eyes you perhaps should be checking other things manually (in other words, don't kill things manually)

On the other hand, it wouldn't hurt / be problematic to add extra bits in, considering all of the code is already here.

@akostadinov
Copy link
Contributor Author

@luke-hill , I assume RubyMine sends SIGTERM. What I was trying to explain is that Ctrl+c (SIGINT) should by default behave the same as SIGTERM. When I hit Ctrl+c I want program to stop, not continue running. Default behaviour is breaking this basic expectation.

wrt clean-up - I assume everybody wants it to have the clean-up most of the times. If somebody wants it skipped, they can do Ctrl+c twice to exit immediately.

@luke-hill
Copy link
Contributor

So your ideal scenario is Ctrl+c says it's stopping (And starts running the After hook), and ctrl+c again during this time completely kills everything? If so I guess this makes a little bit of sense (Assuming at the moment it does nothing after the first ctrl+c).

Although I don't have enough knowledge of this area of the codebase.

@akostadinov
Copy link
Contributor Author

Exactly, thanks.

@luke-hill
Copy link
Contributor

Is this something you think you could work on solo, you want to pair on over github? or you want someone else to take a look.

Just so I know where we need to go forwards on this.

@akostadinov
Copy link
Contributor Author

Not sure about latest code. It was a simple change for the version I reported with (my bad for not writing which exactly version it was).

Is there an agreement that SIGINT and SIGTERM should behave the same? If so I guess it would be easy for me to do it.

@luke-hill
Copy link
Contributor

Honestly not sure. To me they're both interruptions, and in the context of cucumber we don't really "pause" stuff, so my 2cents is they're both kill commands. But I would probably defer to whoever wrote the original code (Probably one of the main members of cucumber)

@tooky
Copy link
Member

tooky commented May 9, 2019

I'm not sure of the history here, but I wonder if the idea is that initially we want to try and execute as cleanly as possible, but if there's something hanging we have a way to just abort.

With the proposed change what happens if the After hook is the problem and Cucumber hangs there?

@akostadinov
Copy link
Contributor Author

@tooky , then you hit Ctrl+C a second time.

@tooky
Copy link
Member

tooky commented May 9, 2019

@akostadinov right - so the UX we're aiming for is:

ctrl-c 👉 cancel the test run where it is, but run after hooks to clean up
ctrl-c again 👉 just stop everything

That seems good to me.

@luke-hill
Copy link
Contributor

I think given Tooky's knowledge of the codebase and that we're in a kind of consensus here with some contributors / collaborators; that's an essential "green-light" to make the changes you want @akostadinov - If you're comfortable doing it.

I'd happily review alongside others

@akostadinov
Copy link
Contributor Author

Thank you, see #1353

@lock
Copy link

lock bot commented Jun 24, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧷 pinned Tells Stalebot not to close this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants