Skip to content

Remove unreachable throw code #39107

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.

Sign up for GitHub

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

Jump to bottom
Closed
BenchmarkingBuffalo wants to merge 1 commit into from
Closed

Remove unreachable throw code #39107

BenchmarkingBuffalo wants to merge 1 commit into from

Conversation

@BenchmarkingBuffalo
Copy link
Contributor

@BenchmarkingBuffalo BenchmarkingBuffalo commented Jan 11, 2024

Remove the dead code after calling 'handleRunFailure'.

No adjustment of tests needed.
Fix #39086

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 11, 2024
wilkinsona
wilkinsona requested changes Jan 11, 2024
View reviewed changes
Copy link
Member

@wilkinsona wilkinsona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Unfortunately, this isn't the change that we want to make. As described in the issue, we want to change handleRunFailure to return the exception and to then have it thrown by the caller. This will eliminate the dead code while continuing to make it clear that the method won't continue to execute.

@BenchmarkingBuffalo
Copy link
Contributor Author

BenchmarkingBuffalo commented Jan 11, 2024

Hi @wilkinsona,
thanks for clarifying.
I will prepare something.

@BenchmarkingBuffalo
Return possibly wrapped exception on 'handleRunFailure'
5132ff4 
Said method returns the exception wrapped as IllegalStateException if necessary.
The if-statements for handling different exceptions use dedicated catch-blocks.
Fix spring-projectsgh-39086
@BenchmarkingBuffalo
Copy link
Contributor Author

BenchmarkingBuffalo commented Jan 11, 2024 •
edited
Loading

Hi @wilkinsona,
I changed the behavior to the one described in the issue.
I also did a small change to the structure of the exception-handling.
This was originally due to a hint from sonar, but I actually like it. For me it is easier to understand looking at multiple catch-statements than a wrapped if-statement distinguishing between the exception types.
If I did not get it right or you would like me to change something in my solution, just hit me up.

P.S. I was not sure, if handleRunFailure is still a good name, because the other handle... methods are all void, but I did not come up with a better name without creating a long one.

@philwebb philwebb added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 11, 2024
@philwebb philwebb added this to the 3.1.x milestone Jan 11, 2024
@philwebb philwebb self-assigned this Jan 11, 2024
@philwebb philwebb mentioned this pull request Jan 11, 2024
Closed
@philwebb philwebb changed the title Remove unreachable throw Remove unreachable throw code Jan 16, 2024
@philwebb philwebb modified the milestones: 3.1.x, 3.1.8 Jan 16, 2024
@philwebb philwebb mentioned this pull request Jan 16, 2024
Closed
philwebb pushed a commit that referenced this pull request Jan 16, 2024
@BenchmarkingBuffalo @philwebb
Remove unreachable throw code
a4ae660 
Improve `SpringApplication` by removing the unreachable throw statement
in favor of returning an exception from `handleRunFailure`. This commit
also removes the if statements in favor of dedicated catch blocks.

See gh-39107
philwebb added a commit that referenced this pull request Jan 16, 2024
@philwebb
Polish 'Remove unreachable throw code'
e23e431 
See gh-39107
@philwebb philwebb closed this in b796447 Jan 16, 2024
@philwebb
Copy link
Member

philwebb commented Jan 16, 2024

Thanks very much @BenchmarkingBuffalo. I made one further tweak by pushing the AbandonedRunException logic into the handleRunFailure method.

@BenchmarkingBuffalo BenchmarkingBuffalo deleted the 39086-remove-dead-code branch January 16, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wilkinsona wilkinsona Awaiting requested review from wilkinsona

Assignees

@philwebb philwebb

Labels

type: task A general task

Projects

None yet

Milestone

3.1.8

Development

Successfully merging this pull request may close these issues.

4 participants

@BenchmarkingBuffalo @philwebb @wilkinsona @spring-projects-issues