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

Add a way to disable adding interrupts. #2689

Open
ioquatix opened this issue Jan 22, 2020 · 15 comments
Open

Add a way to disable adding interrupts. #2689

ioquatix opened this issue Jan 22, 2020 · 15 comments

Comments

@ioquatix
Copy link
Contributor

In code which needs to test fork and other process related functions, it should be possible to disable the interrupt handler, which interferes with code which has it's own signal handlers.

trap_interrupt

Suggest something like config.disable_signal_handlers! or something similar.

@pirj
Copy link
Member

pirj commented Jan 22, 2020

@ioquatix can you please submit a minimal reproduction case that fails with trap_interrupt, but is passing without it?

@JonRowe
Copy link
Member

JonRowe commented Jan 22, 2020

Hm, they carry over to children? I mean we trap our INT for Ctrl C, without it theres no way to hard interrupt us.

@ioquatix
Copy link
Contributor Author

Yes it carries to children.

@ioquatix
Copy link
Contributor Author

ioquatix commented Jan 22, 2020

Here is a working example:

#!/usr/bin/env rspec

RSpec.describe "fork" do
  it "handles interrupt correctly" do
    pid = fork do
      trap(:INT, :DEFAULT)
      sleep
    end

    Process.kill(:INT, pid)
		
    sleep(1)
		
    result = Process.wait(pid, Process::WNOHANG)
    expect(result).to_not be_nil
  end
end

@rspec rspec deleted a comment from ioquatix Jan 22, 2020
@JonRowe
Copy link
Member

JonRowe commented Jan 22, 2020

edit: deleted comment was duplicate code, removed for clarity

Thats less than ideal... but you might be waiting forever with no way to quit if you disabled our handler, I'm not adverse to adding a way to suppress it however...

@JonRowe
Copy link
Member

JonRowe commented Jan 22, 2020

Is it possible to remove them? I vaguely remember issues with puma in a similar vein, I'm wondering if we can provide a method instead, wrap a test inside a "no trap" handler which has a backup timeout kill before restoring the old one?

@ioquatix
Copy link
Contributor Author

If I was building RSpec, I'd want it to be minimally invasive on program state.

Even minitest gets this quite wrong IMHO: minitest/minitest#429

As signals are part of user code state, even if it's not traditionally something that needs to be tested, it does affect the behaviour of user-facing interfaces.

So, my suggestion would be to find a different solution. If the user presses Ctrl-C, the main thread will raise an Interrupt exception by default. I'd suggest you catch this instead.

So:

def run_tests
end

def run_all
  begin
     run_tests
  rescue Interrupt
    # exit and cancel current test
  end
end

@ioquatix
Copy link
Contributor Author

Also, another point, pressing Ctrl-C, I expect a backtrace, but right now it gets swallowed up and often I never see it if the test has actually failed due to some blocking operation - presssing Ctrl-C to cancel the tests doesn't print any output.

@JonRowe
Copy link
Member

JonRowe commented Jan 22, 2020

The expected behaviour is that the first Ctrl-C prints a message but carries on waiting for the current test to finish. The second Ctrl-C causes a hard exit. We do this because it allows cleanup to run for tests that aren't blocked, so in the case of the majority of tests they get a clean exit, but we also allow overriding that as a, no now, type deal.

@ioquatix
Copy link
Contributor Author

If the test will never finish, pressing Ctrl-C a second time exits with no backtrace. Is this intentional?

@JonRowe
Copy link
Member

JonRowe commented Jan 23, 2020

Not intentional, just a side effect of implementation, a second Ctrl-C causes us to exit, there is no backtrace as this is an intended Ruby exit, not a crash. I don't think we could display a stacktrace if we even wanted to...

@ioquatix
Copy link
Contributor Author

I think you should make this behaviour optional and I'd probably immediately disable it in my template Rspec configuration, because I find it so annoying when debugging that exceptions are swallowed and backtraces are not printed.

@JonRowe
Copy link
Member

JonRowe commented Jan 23, 2020

As I said, open to improving this if you want to work on something better, but current behaviour needs to be preserved by default :)

@ioquatix
Copy link
Contributor Author

Okay, I'll make a PR.

@ioquatix
Copy link
Contributor Author

#2722

Here is the PR, can you please check it? Thanks.

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

No branches or pull requests

3 participants