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

Fix execution hangs #657

Merged
merged 7 commits into from
Mar 9, 2020
Merged

Fix execution hangs #657

merged 7 commits into from
Mar 9, 2020

Conversation

pawelpasterz
Copy link
Contributor

@pawelpasterz pawelpasterz commented Mar 8, 2020

Fixes #649

This PR contains commit 2b92fec which is only small refactor which touches quite big number of files. Please, skip this commit when reviewing.

Changes:

  • wrap CommandLine(Main()).execute(*args) with exitProcess(...) which closes jvm (this is workaround for bugsnag bug related to thread pool executor"
  • add unit tests

Checklist

  • Documented
  • Unit tested
  • release_notes.md updated

@codecov-io
Copy link

codecov-io commented Mar 8, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@dd89178). Click here to learn what that means.
The diff coverage is 59.21%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #657   +/-   ##
=========================================
  Coverage          ?   77.62%           
  Complexity        ?      641           
=========================================
  Files             ?       88           
  Lines             ?     2538           
  Branches          ?      333           
=========================================
  Hits              ?     1970           
  Misses            ?      354           
  Partials          ?      214

bootstraponline
bootstraponline previously approved these changes Mar 8, 2020
@pawelpasterz
Copy link
Contributor Author

@bootstraponline @remkop
Thanks for comments, I've added changes you requested, please verify.
Thanks :)

@bootstraponline
Copy link
Contributor

Looks great! Approved. Thanks for updating and adding the test.

@pawelpasterz pawelpasterz merged commit 71314ac into master Mar 9, 2020
@pawelpasterz pawelpasterz deleted the fix-execution-hangs branch March 9, 2020 20:56
@remkop
Copy link

remkop commented Mar 9, 2020

The classpath in UtilsTest still has the dummy value classpath...picocli-4.2.0.jar. So that process will die immediately. It won’t hang.

You can get a better classpath from the value of system property "java.class.path".
Sorry if the example was misleading.

@bootstraponline
Copy link
Contributor

Thanks for the follow up clarification! I opened #662 to track this work.

@pawelpasterz
Copy link
Contributor Author

@remkop Omg, I totally missed that ... shame on me. Thanks for catching this!
Here is fix #663 I also added additional verification logic if process was started.
cc @bootstraponline

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

Successfully merging this pull request may close these issues.

Execution hangs when exceptions are thrown
5 participants