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

Issue 703: making prevention of System.exit() calls configurable. #707

Merged
merged 1 commit into from Apr 19, 2015

Conversation

Projects
None yet
4 participants
@benhamidene
Contributor

benhamidene commented Apr 5, 2015

Introduced new system property prevent.system.exit to control the activation of the SystemExitSecurityManager. Default behaviour is that the SystemExitSecurityManager is activated if the property is not found

@amolenaar

This comment has been minimized.

Collaborator

amolenaar commented Apr 13, 2015

This looks good. Thanks. Only when I tried an "ant release", the test process was abruptly terminated. It seems like the test case fitnesse.junit.FitNesseSuiteExampleTest is failing. Can you have a look please?

@benhamidene

This comment has been minimized.

Contributor

benhamidene commented Apr 14, 2015

I have fixed the problem. All ant tasks should run now without errors

@amolenaar

This comment has been minimized.

Collaborator

amolenaar commented Apr 14, 2015

Shouldn't the default be to prevent system exit? Since that's the old behaviour.

@benhamidene

This comment has been minimized.

Contributor

benhamidene commented Apr 14, 2015

well I am a fan of failing fast .. and clean coding:)

And I have changed the behaviour only for test run as Junit-Tests , not from a fitnesse server.

I can not see why we should tolerate/handle calling System.exit in Junit-Tests. If a developer makes such a thing then he should notice it soon and should fix it. If it is part of the test then it could be set for that test case.

I don´t think it is critical if the test run is interrupted when a System.exit() is called in a junit test but it might be when the test is run from the UI of a central FitNesse instance.

@bfassbender

This comment has been minimized.

bfassbender commented Apr 15, 2015

I'm also interested in this pull request as I am experiencing the same performance issue when running tests for i/o intensive applications.

And I second benhamidene's optinion on preferring failing fast and better runtime performance in case of the junit-test runner instead of defaulting to the setting for the wiki-mode.

@benhamidene

This comment has been minimized.

Contributor

benhamidene commented Apr 16, 2015

Hi amolenaar,

any chances to merge this commit ? not convinced by my answer :) ?

@amolenaar

This comment has been minimized.

Collaborator

amolenaar commented Apr 18, 2015

:) I think you're right. The only doubt I have is that the default behaviours differ when executing a suite from the wiki vs. executing the suite from jUnit.

@mgaertne

This comment has been minimized.

Collaborator

mgaertne commented Apr 18, 2015

I thought that I should provide some background information on why I initially introduced that feature. At a time there were lots of folks asking why FitNesse wasn't providing information on why a test was aborted. Most of these cases ended up as "production code called System.exit() for error handling". Eventually the amount of questions and people blaming FitNesse for the problems was so high, that we incorporated this feature. It seemed to be a good idea at that time.

Now, be aware that changing the default behavior may have people coming back for the same reasons.

Best Markus

@benhamidene

This comment has been minimized.

Contributor

benhamidene commented Apr 18, 2015

Hi ,
I understand your arguments .But.

  • It is a fact the SystemExitSecurityManager is responsible for massive performance leaks. We have a spring based integration test whose execution duration went down from 380 sec (!!!) down to 25 sec after removing the SystemExitSecurityManager .
  • My commit changes the default behavior exclusively for tests run by the JUnit-Runner. The behavior for test run from an FitNesse instance stayd unchanged.
  • I am still convinced that using System.exit() in an application or a test is not a good practice. If it has to be used than it is possible to activate the SystemExitSecurityManager as I made for the PreventExit Test. I would recommend to consider that as the exception and not the rule.

If you are not convinced and because the performance issue is very critical for us I could turn back the default behavior of the JUnitRunner and make a separate pull request for it that could be voted .

@amolenaar

This comment has been minimized.

Collaborator

amolenaar commented Apr 18, 2015

I agree with your reasoning as far as point 1 and 2 go. Considering point 3, since we're executing tests in a separate process, it's harder to figure out what went wrong.

It thing it would be fine if FitNesseRunner.shouldPreventSystemExit() returned true by default, since that will be the slower, but more secure option. Power users like yourself can add the annotated when needed. Also, there should be no difference in execution, depending on whether your executing tests from the web interface or from JUnit.

@mgaertne

This comment has been minimized.

Collaborator

mgaertne commented Apr 19, 2015

I understand the reasons why you changed the behavior. I also agree that System.exit() in applications is bad. Nevertheless people are doing terrible things out there, and I wanted to make you aware of the side-effects to your changes.

I certainly agree with Arjan that the behavior should be consistent throughout the application no matter how you run it.

Best Markus

Anis Ben Hamidene
Issue 703: making prevention of System.exit() calls configurable.
Introduced new property prevent.system.exit.
@benhamidene

This comment has been minimized.

Contributor

benhamidene commented Apr 19, 2015

OK. You win :)

I have updated the commit and added a short description for the property. Ant builds run without error. It should be ready to go now ;)

Best regards .
Anis

@amolenaar amolenaar merged commit be19315 into unclebob:master Apr 19, 2015

amolenaar added a commit that referenced this pull request Apr 19, 2015

Merge pull request #707 from benhamidene/issue703
Issue 703: making prevention of System.exit() calls configurable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment