Allow optional trapping of interrupts, and restore previous interrupt… #2722
Conversation
|
@JonRowe what's the chance of getting this released soonish? (provided you are happy with it). |
ioquatix
force-pushed
the
with-interrupts
branch
from
7657558
to
bd88112
Compare
There was a problem hiding this comment.
Thanks for tackling this @ioquatix I'd like to suggest a different approach if thats ok, where by we capture the original handler before we trap the interrupts, and then add a method to restore it permanently in config rather than wrapping with a yield, this is because setup also needs to handle interrupts by default.
| @@ -230,6 +230,10 @@ def fail_fast=(value) | |||
| end | |||
| end | |||
|
|
|||
| # @macro trap_interrupt | |||
| # Default: `true`. | |||
| add_setting :trap_interrupt | |||
There was a problem hiding this comment.
I think I'd rather handle this like we handle zero monkey patching mode, that is that theres a method which clears our event handler and restores the previous one.
e.g.
def clear_interrupt_handler!
Signal.trap(:INT, RSpec.world.original_interrupt_handler)
end
I don't think we need to conditionally restore ours at any point, it should be a once only deal, you're either going to want this or not.
There was a problem hiding this comment.
# This is used internally by RSpec to run a suite, but is available
# for use by any other automation tool.
In your own comment, you say other tools may use this entry point.
So, IMHO, it should correctly set and restore interrupt handlers.
it should be a once only deal, you're either going to want this or not.
What happens if the user calls this method more than once?
What are the invariants regarding signal handling that we want?
There was a problem hiding this comment.
What happens if the user calls this method more than once?
I'm ok with it being a no-op, you've called it, we've removed our handlers, you call it again theres nothing for us to do.
There was a problem hiding this comment.
No, I mean what if the user calls Runner.run more than once, maybe with different options?
There was a problem hiding this comment.
Ah I see, in this case as its a global option it's behaviour would be permanent, (although I'm ok if you want to add a way to undo it too).
| @@ -62,7 +62,6 @@ def self.invoke | |||
| # or the configured failure exit code (1 by default) if specs | |||
| # failed. | |||
| def self.run(args, err=$stderr, out=$stdout) | |||
| trap_interrupt | |||
There was a problem hiding this comment.
I don't want to remove this, its important we trap by default during setup in case its expensive, this is what the majority of our users have come to expect, the suggestion above would help with this.
There was a problem hiding this comment.
What parts of setup are expensive?
def self.run(args, err=$stderr, out=$stdout)
options = ConfigurationOptions.new(args)
if options.options[:runner]
options.options[:runner].call(options, err, out)
else
new(options).run(err, out)
end
end
Are you concerned about options.options[:runner].call(options, err, out)?
There was a problem hiding this comment.
I'm concerned with setup(err, out) which loads all the spec files, and thus any requires within them (that often trigger rails booting).
There was a problem hiding this comment.
If no tests have run, what is the point of handling interrupts during that time?
There was a problem hiding this comment.
If no tests have run, what is the point of handling interrupts during that time?
To preserve the behaviour that currently exists.
There was a problem hiding this comment.
Right, that's a fair point. But what I'm asking is does that behaviour make sense? From what I can tell, this feature is designed to allow breaking out of tests gracefully while still printing some status/test report. But there is no such report when breaking out of the setup phase because no tests have been run.
Yes it makes sense, it tells people RSpec has not frozen whilst say, Rails is booting, without cancelling a build.
I would suggest you consider the following:
- The proposed implementation solves the issue, and all specs are passing with no global state.
The proposed implementation is a breaking change in behaviour and cannot be merged as is.
- It has a well defined behaviour in terms of restoring the signal handling which is absolutely critical if you want to embed this testing framework into something else.
Whilst we are mindful of how we can be integrated into other tools, our primary usage is as a standalone command line tool, the existing implementation has no signal handling if you instantiate the runner yourself, it is only our short cut run function which does.
The counter proposal you suggested involves more global state and doesn't correctly set the signal handling back to the original state.
To me, what concerns me about your proposal is:It has more moving parts, some of which are global state.
It doesn't correctly reset the signal handling after exiting the test runner.
Can we find a solution to those problems?If you are concerned only about the setup, why don't we wrap that into the interrupt handling?
Signal handlers are global state, there is no getting around that. Your implementation has global state and is hoping that there is no other ruby code in this process or spawned from this process that outlasts your block. I believe that the handler will be passed down to forked children and similarly not reset?
I'm open to having both an install method and a remove method, called before and after the class run method. It just needs to replicate the existing behaviour.
The difficulty is that the handler is global, needs to be installed before setup is run, which might then remove it. This is similar to how our global monkey patching works, for the same reason.
There was a problem hiding this comment.
e.g. here would call install_signal_handler!, line 71 would call remove_signal_handler! (taking care to return the result of the runner call / new.run call).
There was a problem hiding this comment.
Yes, you are right, this is modifying global state. However if you wrap your code like this:
begin
previous = Signal.trap(...)
# user code
ensure
Signal.trap(..., previous)
end
It will be correctly enabled and restored.
Regarding child processes, that is one reason why RSpec should not modify signal handlers at all. So setting it and then setting it back to what it was when it was set the first time, it is not ideal (it could have been changed in the mean time, so setting it back might actually do the wrong thing). It should simply not set it in the first place. The goal of this PR is to provide a way to tell RSpec to NOT call Signal.trap.
Yes it makes sense, it tells people RSpec has not frozen whilst say, Rails is booting, without cancelling a build.
So someone runs RSpec, which loads rails, immediately hits Ctrl-C and it waits for the entire boot process, then exits with an empty summary. I'm not sure why that is preferable over just exiting right away. Can you explain it?
There was a problem hiding this comment.
The goal of this PR is to provide a way to tell RSpec to NOT call Signal.trap.
That is not happening. It is not possible to configure RSpec before we need to install the handler. Chicken and egg.
So someone runs RSpec, which loads rails, immediately hits Ctrl-C and it waits for the entire boot process, then exits with an empty summary. I'm not sure why that is preferable over just exiting right away. Can you explain it?
If that expensive process is manipulating data it leaves it in a consistent state rather than interrupting half way through.
However why it is preferable or not is personal choice, its not relevant.
What is relevant is that changing the behaviour is from our interpretation a breaking change (which I have mentioned before) in semantic versioning terms, and thus is not possible before RSpec 4 even if there was majority consent for this change.
There was a problem hiding this comment.
I see.
Well, if users correctly handle interrupts, this should not be an issue. But if that's your decision, I can respect it.
So what about waiting for RSpec 4? I'm fine with that if you think it makes sense.
Because what I want is a way to stop RSpec from registering signal handlers. I'm not interested in the counter proposal you made, it's probably not going to solve my core issues.
… handler after running tests.
ioquatix
force-pushed
the
with-interrupts
branch
from
c655899
to
199016c
Compare
|
Sorry @ioquatix I know this will be a frustrating PR for you, however I must stress that we cannot change the current outside behaviour, and thus can only allow optional removal of the signal trap handlers. I'm happy for you to refactor this PR in two ways:
Unfortunately it is not possible to never install the handler, because the handler is installed before config is parsed, so there is no way to know in advance if the user would like to not use it and anything else would violate our interpretation of semantic versioning by being a breaking change. Let me know if you're happy to make these changes or close if you feel you don't have time to improve this further. |
|
If you want to take another stab at this for RSpec 4, I'm fine with that. |
|
I just hit this requirement, I have a test that never finishes after a Ruby upgrade and I want to run some profiling, but I've not been able to convince Rspec to dump me out in a context where I can do the profiling I need to do. |
|
@ioquatix If you'd like to revive this, we have a |
|
Basically, I don't think you should ever trap interrupts. If this is acceptable for RSpec 4 I am happy to update the PR. |
|
I'm still hesistant to remove the default useful behaviour, what we can do in RSpec 4 is delay it and say it doesn't apply to setup to give the user a chance to surpress it, but so far there hasn't been a large amount of support for making further changes. |
|
I'm not sure the default behaviour is particularly useful - in fact the behaviour is incredibly frustrating when dealing with hanging or otherwise borked tests. |
|
In the case of hanging or otherwise borked tests you'd have to hit ctrl-c (or otherwise signal) to quit, so having to hit ctrl-c twice given its one more key press hardly seems incredibly frustrating, to me at least, obviously I cannot speak for how frustrating that is to yourself or others. However what this behaviour allows RSpec to do is clean up in the much more common "I want to quit mid run" scenario, it ensures hooks etc are run allowing cleanup of databases, files and other such things to happen hoping to leave a clean slate. My concern about making this opt in rather than opt out is that this would be incredibly surprising to people who are used to be able to "just quit" mid run and have RSpec be able to run again without manual clean up. This is less of a problem for libraries and when using transactional tests, but not everyone does that and there is a huge amount of Rails apps I wouldn't trust without database cleanup. |
It doesn't print any backtrace like a normal interrupt. Try it. This isn't a problem of pressing Ctrl-C twice, it's the fact that there is no backtrace or other error details. |
Thats something we could fix |
That would be awesome. |
… handler after running tests.
My suggestion is to make this the default for RSpec 4.0 because I think it's best for RSpec to be minimally intrusive on tests. What I mean by that is some code may wish to test the behaviour of SIGINT or depend on it's behaviour. The less global state RSpec is changing/manipulating, the better IMHO.