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

Timeout stops threads if they are uninterruptible. #811

Closed

Conversation

aslakhellesoy
Copy link
Contributor

If a thread does not respond to interrupt() we'll
try to stop() it after twice the specified timeout.

This uses the deprecated Thread.stop() method, but
for a testing tool like Cucumber that should be ok.

Ref #343.

If a thread does not respond to interrupt() we'll
try to stop() it after twice the specified timeout.

This uses the deprecated Thread.stop() method, but
for a testing tool like Cucumber that should be ok.

Ref #343.
@aslakhellesoy
Copy link
Contributor Author

@brasmusson @os97673 let me know what you think about this approach to deal with the situation described in #343.

Acceptable?

@paul8620
Copy link

True the junit and testng solution works, but if I have a scenario in witch I know how much time a request should work then... I don't want that increased to double the timeout

@paul8620
Copy link

@aslakhellesoy
Copy link
Contributor Author

@paul8620 please send your fix in a pull request. The code you pasted doesn't seem to solve the problem,

@dkowis
Copy link
Member

dkowis commented Dec 18, 2014

So the infinite loop without any sleep or anything in there is a terrible thing, that will spin so tightly it'll use up 100% of all the CPU on any system. There's some flaws in there...

@aslakhellesoy
Copy link
Contributor Author

@dkowis yes it's a terrible thing, and cucumber should be able to time out terrible things like that. junit can. Try this:

@Test(timeout = 20)
public void yep() {
    while(true);
}

@brasmusson
Copy link
Contributor

I can't say I'm that familiar with all the intricate details of the thread handling in Java. This post makes a strong case that Thread.interrupt should be the first option, and only if the running step definition does not play nicely with Thread.interrupt, Thread.stop could be tried.

It seems like not even Thread.stop will always work, however (quote from the post):
"It should be noted that in all situations where a waiting thread doesn't respond to Thread.interrupt, it wouldn't respond to Thread.stop either. Such cases include deliberate denial-of-service attacks, and I/O operations for which thread.stop and thread.interrupt do not work properly." Maybe the last resort should be to log that cucumber tried to stop the step definition but couldn't.

@aslakhellesoy
Copy link
Contributor Author

I'm not a fan of using Thread.stop either. Does anyone have the time and energy to find out how JUnit deals with it? The snippet I posted above is definitely not interruptible with Thread.interrupt.

@brasmusson
Copy link
Contributor

JUnit uses Thread.interrupt see Javadoc, that does not necessarily stop the test, but as each test is launched in a separate thread, JUnit can still mark the test as failed, and launch the next test. This gist (executed from Eclipse on Linux) demonstrates that tests continues to run after they have been interrupted due to the timeout.

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jun 11, 2017

If we want to do things the junit way then it seems to me that the timeout is done at the wrong level.

We should start a worker thread for the calls in Runner.runPickle. This would also require that we wrap the EventBus to prevent slow pickles from sending events after they've had a timeout and listen to step finished events to reset the timer.

If the Pickle refused to die the worker thread it is on can then safely be left to spin while the main thread continues with the next Pickle.

@mpkorstanje mpkorstanje deleted the stop-uninterruptible-threads-on-timeout branch October 11, 2017 19:38
@lock
Copy link

lock bot commented Oct 24, 2018

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 Oct 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants