Spring Batch
  1. Spring Batch
  2. BATCH-1668

Check for existing transaction when job is started (and fail if present by default)

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Complete
    • Affects Version/s: 2.1.5
    • Fix Version/s: 2.1.6
    • Component/s: Core
    • Labels:
      None

      Description

      I have a suspicion that this may be configuration related, but the outcome is certainly something I'd consider a bug...

      This is in a JTA context, and I'm assuming that this is relevant...

      I have a batch job which integration testing with 3 items, and a chunk size of 2.

      The first chunk is processed, and then I get a deadlock at semaphore.acquire() at TaskletStep.java:390

      The reason for this, is that afterCompletion() hasn't been called, because the transaction didn't commit, because it wasn't a new one.

      Seems to me that if this is a configuration issue, then I shouldn't be able to create this scenario...

      Does Spring Batch really intend to move on to the next chunk at this point?

        Activity

        Hide
        Dave Syer added a comment -

        You said it's an integration test. Does that mean you wrapped the job execution in a transaction (e.g. with @Transactional)? This is not supposed to work (whole job transaction), and the effect would be as you describe.

        Show
        Dave Syer added a comment - You said it's an integration test. Does that mean you wrapped the job execution in a transaction (e.g. with @Transactional)? This is not supposed to work (whole job transaction), and the effect would be as you describe.
        Hide
        Neale Upstone added a comment -

        Correct. We were using @Transactional, and have now moved to commit and assert that no transactions are active.

        Given what you say, then it seems that an active transaction on the transaction manager being used by batch is an illegal state, so perhaps you might find an appropriate place to add:

        transactionManager.getTransaction(new DefaultTransactionDefinition(TransactionDefinition.PROPAGATION_NEVER));
        
        Show
        Neale Upstone added a comment - Correct. We were using @Transactional, and have now moved to commit and assert that no transactions are active. Given what you say, then it seems that an active transaction on the transaction manager being used by batch is an illegal state, so perhaps you might find an appropriate place to add: transactionManager.getTransaction( new DefaultTransactionDefinition(TransactionDefinition.PROPAGATION_NEVER));
        Hide
        Dave Syer added a comment -

        That's not a bad idea - TaskletStep might be able to do it. I'm not sure if the transaction object has to be cleaned up afterwards (suspect it does). If you want to try it out and let us know, that would be great.

        Show
        Dave Syer added a comment - That's not a bad idea - TaskletStep might be able to do it. I'm not sure if the transaction object has to be cleaned up afterwards (suspect it does). If you want to try it out and let us know, that would be great.
        Hide
        Neale Upstone added a comment -

        Forked and taking a look.

        Agree on cleanup. Using TransactionTemplate would be a safer here.

        Let's be clear on the contract and point(s) at which no transaction is allowed to be active though.

        I think what you're suggesting is that we ensure our transaction created in TaskletStep is new and that no previous one existed. There isn't a transaction definition for this (maybe a JIRA for JH .

        I'd suggest we just ensure on Job startup that there is no transaction active.

        Show
        Neale Upstone added a comment - Forked and taking a look. Agree on cleanup. Using TransactionTemplate would be a safer here. Let's be clear on the contract and point(s) at which no transaction is allowed to be active though. I think what you're suggesting is that we ensure our transaction created in TaskletStep is new and that no previous one existed. There isn't a transaction definition for this (maybe a JIRA for JH . I'd suggest we just ensure on Job startup that there is no transaction active.
        Hide
        Neale Upstone added a comment -

        Been mulling on this while shifting boxes...

        I'd toyed with the idea that it's just a simple case that TaskletStep needs its own transaction, and therefore a simple REQUIRES_NEW instead of default REQUIRED. I think REQUIRES_NEW in TaskletStep would be more valid, as the reason for the problem I got was because REQUIRED prompted an attempt to join the existing transaction, which I think is what tripped up the lock manipulation in your transaction sync. I'll have a look at whether REQUIRES_NEW will do what I was trying to do in the first place, which was to be able to rollback the whole job.

        Show
        Neale Upstone added a comment - Been mulling on this while shifting boxes... I'd toyed with the idea that it's just a simple case that TaskletStep needs its own transaction, and therefore a simple REQUIRES_NEW instead of default REQUIRED. I think REQUIRES_NEW in TaskletStep would be more valid, as the reason for the problem I got was because REQUIRED prompted an attempt to join the existing transaction, which I think is what tripped up the lock manipulation in your transaction sync. I'll have a look at whether REQUIRES_NEW will do what I was trying to do in the first place, which was to be able to rollback the whole job.
        Hide
        Dave Syer added a comment -

        REQUIRES_NEW for the Tasklet will get around the original problem, but will potentially lead to other problems because the JobRepository data might not be consistent between the existing transaction (whose commits are not visible) and the one in the Tasklet. The best you could hope for would be a non restartable Job that would work on a sunny day. The worst would be an unrecoverable JobRepository failure.

        Testing for an existing transaction in the JobLauncher would be an option. Easiest way to do it with no side effects is through the JobRepository by changing the propagation level in the createJobExecution() advice (which is added in the JobRepositoryFactoryBean).

        Show
        Dave Syer added a comment - REQUIRES_NEW for the Tasklet will get around the original problem, but will potentially lead to other problems because the JobRepository data might not be consistent between the existing transaction (whose commits are not visible) and the one in the Tasklet. The best you could hope for would be a non restartable Job that would work on a sunny day. The worst would be an unrecoverable JobRepository failure. Testing for an existing transaction in the JobLauncher would be an option. Easiest way to do it with no side effects is through the JobRepository by changing the propagation level in the createJobExecution() advice (which is added in the JobRepositoryFactoryBean).
        Hide
        Dave Syer added a comment -

        I added a check for an ongoing transaction when the JobExecution is created (see AbstractJobRepositoryFactoryBean). The old behaviour is available if anyone needs it via a flag (validateTransactionState).

        Show
        Dave Syer added a comment - I added a check for an ongoing transaction when the JobExecution is created (see AbstractJobRepositoryFactoryBean). The old behaviour is available if anyone needs it via a flag (validateTransactionState).
        Hide
        Neale Upstone added a comment -

        Looks good to me.

        Haven't re-tested my scenario, but code review was educational. I see you've also now narrowed the advice to create* which looks it was intended to be all along.

        Many thanks.

        Show
        Neale Upstone added a comment - Looks good to me. Haven't re-tested my scenario, but code review was educational. I see you've also now narrowed the advice to create* which looks it was intended to be all along. Many thanks.

          People

          • Assignee:
            Dave Syer
            Reporter:
            Neale Upstone
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: