A failure in ItemStream.close does not fail the step

[spring-projects-issues]

Valentin Ozanne opened BATCH-1864 and commented

Hi all,

I am developing batchs with Spring Batch for about a year and I would like to discuss about a possible evolution in close() handling of a stream.

What I could see :

• Writers like FlatFileItemWriter uses close() method to provide FooterCallback support.
• When an exception occurred during close() operation, the error is only logged and the step is marked as COMPLETED. It's ever not possible to catch exception inside a stepListeners because close() method is called after listeners. I personally consider this as a bug but I would like to

What I propose : Inside the AbstractStep.execute()

• Moving close execution before listeners execution
• Adding ExitStatus handling inside catch bloc of close() execution.

Could you give me a feedback on this proposition? If feedbacks are ok I may try a pull request on this.

---

Affects: 2.1.8

1 votes, 3 watchers

[spring-projects-issues]

Dave Syer commented

Thanks for the suggestion. I believe, however, that ItemStream.close() is called at a very precise point in the step execution for a particular reason, so we might need to think a bit harder about it if you want to make changes. The original design allows for ItemStream.close() to do clean up of resources that were used in the step, and nothing more. The view taken at the time was that an exception in that case would be after all the releveant business processing so it should not cause a failure of the step. What kind of scenario do you have where that is not the case, and why could it not be handled by a listener?

[spring-projects-issues]

Valentin Ozanne commented

Hi Dave, Thank you for your comment. Here is a typical scenario: When I use a FlatFileItemWriter, I want to write footer (a special record at the end of the file). For doing this, the writer calls a FooterCallback during his close(). An exception may be occurred during a FooterCallback because compute Footer may be a "complex" business operation. If an exception occurs, I can’t catch it inside a step listener because the step listeners are called before the close().

May be the FooterCallback should be called inside a listener instead. But In any cases, don’t you think that it would be better if an exception during closing resource could be catch during step Listeners if needed?

[spring-projects-issues]

Valentin Ozanne commented

Hi all,

What do you think about this possible improvement ?

Thanks in advance for your feebacks.

Valentin

[spring-projects-issues]

Dave Syer commented

I already said roughly what I think. If you want to make a change, feel free to send a pull request, but make sure you test it (and run all the tests in all the projects with -P all). A change in the semantics of callbacks would probably not be appropriate for a point release either, so don't be surprised if we try to get 2.1.9 out before your change gets any attention.

[spring-projects-issues]

Dave Syer commented

See also BATCH-1697. There are some arguments there about why close() is called where it is. I believe this needs some careful thought.

[spring-projects-issues]

Dave Syer commented

See also BATCH-1799

[spring-projects-issues]

Michael Bannister commented

Just found this, having added a comment on BATCH-1697 to the effect that StaxEventItemWriter writes its closing tags in close() and therefore really needs to complete successfully. I've seen an IOException here when running out of disk space, which means my job gets marked COMPLETED but the XML file is no good. I'd be happy to try and contribute to the efforts to fix if we can agree a direction...

[liorderei]

HI any update on that? I'm facing same issue. I wrote an itemwriter that writes to S3 using multipart, only when closing the resource it sends a complete upload request to AWS. I set it in close(), but when the upload fails, the job still completed

[fmbenhassine]

@liorderei If it were up to me, I would upload the file in a different step or in a listener, and use close only to close the resource. As mentioned by D. Syer in a previous comment: The original design allows for ItemStream.close() to do clean up of resources that were used in the step, and nothing more. Therefore, uploading the file in close is not appropriate IMO.

Any exception in close should endup in the failure exceptions list of the step execution: https://github.com/spring-projects/spring-batch/blob/df5e8e1686ac27e0d2ffa6131bb2e515d0e40ed5/spring-batch-core/src/main/java/org/springframework/batch/core/step/AbstractStep.java#L312-L319. So while it won't be possible to detect that in a StepExecutionListener#afterStep as it is called before close, I think it should be possible to do in a subsequent step. This is not convenient, but should work fine.

I believe we can make the step fail in that case (by adding stepExecution.setStatus(BatchStatus.FAILED);exitStatus = exitStatus.and(ExitStatus.FAILED); in that catch block (similar to https://github.com/spring-projects/spring-batch/blob/df5e8e1686ac27e0d2ffa6131bb2e515d0e40ed5/spring-batch-core/src/main/java/org/springframework/batch/core/step/AbstractStep.java#L299-L310, but I am not sure this change would be without side-effects. Probably to try out in a major release.