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

SystemCommandTasklet does not propagate errors #4483

Closed
cachescrubber opened this issue Nov 9, 2023 · 5 comments
Closed

SystemCommandTasklet does not propagate errors #4483

cachescrubber opened this issue Nov 9, 2023 · 5 comments
Labels
for: backport-to-5.0.x Issues that will be back-ported to the 5.0.x line for: backport-to-5.1.x Issues that will be back-ported to the 5.1.x line has: minimal-example Bug reports that provide a minimal complete reproducible example in: core type: bug
Milestone

Comments

@cachescrubber
Copy link

cachescrubber commented Nov 9, 2023

SystemCommandTasklet does not propagate errors

If a command executed by SystemCommandTasklet returns a non zero exit code, the system exit code is mapped to ExitStatus.FAILED. However, the overall step execution status is BatchStatus.COMPLETED, meaning the step (and the job) is completed successfully.

Environment
spring-batch 5.0.3

Steps to reproduce
Create a Job with a SystemCommandTasklet where the system command executed will return a non zero exit code.

Expected behavior
The error should be propagated to the Step, and ultimately to the overall Job execution status.

@cachescrubber cachescrubber added status: waiting-for-triage Issues that we did not analyse yet type: bug labels Nov 9, 2023
@cachescrubber
Copy link
Author

Example Project: https://github.com/cachescrubber/spring-batch-demo-4483

@fmbenhassine fmbenhassine added in: core has: minimal-example Bug reports that provide a minimal complete reproducible example and removed status: waiting-for-triage Issues that we did not analyse yet labels Nov 17, 2023
@hyosyung
Copy link

hyosyung commented Jan 5, 2024

Hi team, I want to work for this issue. Can I?

@fmbenhassine
Copy link
Contributor

@hyosyung Sure! Thank you for your offer to help. Contributions are welcome!

@fmbenhassine
Copy link
Contributor

@hyosyung Have you managed to work on this issue?

I think the way to address this is to throw an exception if the exit code mapper maps the exit code to a failed status, something like:

	if (systemCommandTask.isDone()) {
-		contribution.setExitStatus(systemProcessExitCodeMapper.getExitStatus(systemCommandTask.get()));
-		return RepeatStatus.FINISHED;
+		Integer exitCode = systemCommandTask.get();
+		ExitStatus exitStatus = systemProcessExitCodeMapper.getExitStatus(exitCode);
+		if (ExitStatus.FAILED.equals(exitStatus)) {
+			throw new SystemCommandException("Execution of system command failed with exit code " + exitCode);
+		}
+		else {
+			contribution.setExitStatus(exitStatus);
+			return RepeatStatus.FINISHED;
+		}
	}

This way, the step will be marked as failed correctly. Wdyt? Please let me know if you can contribute a PR along these lines, otherwise I will take care of the fix. Thank you upfront.

@fmbenhassine fmbenhassine added this to the 5.2.0 milestone Feb 15, 2024
@fmbenhassine fmbenhassine added for: backport-to-5.0.x Issues that will be back-ported to the 5.0.x line for: backport-to-5.1.x Issues that will be back-ported to the 5.1.x line labels Feb 15, 2024
injae-kim added a commit to injae-kim/spring-batch that referenced this issue Mar 12, 2024
@injae-kim
Copy link
Contributor

Hi @fmbenhassine , based on your suggestion I create fix PR #4566 PTAL :)

Thank you for detailed guide!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: backport-to-5.0.x Issues that will be back-ported to the 5.0.x line for: backport-to-5.1.x Issues that will be back-ported to the 5.1.x line has: minimal-example Bug reports that provide a minimal complete reproducible example in: core type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants