Is identical error object check necessary here?

[jjb]

The test suite passes with this change (just a demo): https://github.com/jjb/timeout/pull/6/files

If that's acceptable, then the code can be cleaned up like so, with the benefits of being more readable and also creating fewer objects per invocation: https://github.com/jjb/timeout/pull/5/files

If it's not acceptable, I could make a PR which adds a test.

[eregon]

The check is meant to ensure it's the correct ExitException we raised that we see, not another one, and only translate to Timeout::Error if so.

Notably for cases of nesting Timeout.timeout calls and the outer timeout is smaller than the inner timeout, the inner timeout call should not raise, only the outer one.
The test suite should fail for this change, it means we are missing a test for that case.

[eregon]

We have

timeout/test/test_timeout.rb

Lines 53 to 64 in 03873a9

	def test_nested_timeout
	a = nil
	assert_raise(Timeout::Error) do
	Timeout.timeout(0.1) {
	Timeout.timeout(1) {
	nil while true
	}
	a = 1
	}
	end
	assert_nil a
	end

but it's missing a check to ensure the inner timeout calls does not raise Timeout::Error/that it raises ExitException.

[jjb]

Great

here are a couple tests i threw together

the second one does fail with the diff discussed above

not sure how close this is to what you have in mind for what should be tested

im not sure if there is a "more real world" scenario - if outer is less time than inner then there's no way inner would raise that specific class, the user would need to in their code, which is very edge case, not sure if that's specifically what we're guarding against.

any feedback welcome!

https://github.com/ruby/timeout/pull/39/files

[eregon]

Tests added in #39 so I think we can close this.

[jjb]

confirmed that with new tests, my PRs in original post above are no longer green

jjb#5

jjb#6