Uploaded image for project: 'Spring Framework'
  1. Spring Framework
  2. SPR-15895

SimpleAsyncTaskExecutor not respect ConcurrencyThrottleSupport.NO_CONCURRENCY limit

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Complete
    • Affects Version/s: 4.3.10
    • Fix Version/s: 4.3.11, 5.0 RC4
    • Component/s: Core
    • Labels:
      None
    • Last commented by a User:
      false

      Description

      When I set the SimpleAsyncTaskExecutor setConcurrencyLimit with value ConcurrencyThrottleSupport.NO_CONCURRENCY (i.e. value = 0), the execute of SimpleAsyncTaskExecutor will not take NO_CONCURRENCY in consideration. The Runnable will always be executed in all cases.

      SimpleAsyncTaskExecutor.java

      	public void execute(Runnable task, long startTimeout) {
      		Assert.notNull(task, "Runnable must not be null");
      		Runnable taskToUse = (this.taskDecorator != null ? this.taskDecorator.decorate(task) : task);
      		if (isThrottleActive() && startTimeout > TIMEOUT_IMMEDIATE) {
      			this.concurrencyThrottle.beforeAccess();
      			doExecute(new ConcurrencyThrottlingRunnable(taskToUse));
      		}
      		else {
      			doExecute(taskToUse);
      		}
      	}
      

      The root cause is isThrottleActive() will always returns false as the concurrent limit = 0 in this case, hence ConcurrencyThrottleAdapter will not be processed.

      This is also the reason why the 1st test case SimpleAsyncTaskExecutorTests cannotExecuteWhenConcurrencyIsSwitchedOff is disabled by the SPF committer.

      The current workaround is SimpleAsyncTaskExecutor.setConcurrencyLimit(1).

      BTW, NO_CONCURRENCY = 0 is somehow wrong in definition. No currency should means there should be one invocation of method at a time, other invocations should be waited. That means NO_CONCURRENCY should have a value of 1 instead.

        Issue Links

          Activity

          Hide
          juergen.hoeller Juergen Hoeller added a comment -

          NO_CONCURRENCY - as originally defined in ConcurrencyThrottleSupport - actually means no entrance to the monitor at all, so in this case no async execution at all. In that sense, 0 is the right value for it... but there is nevertheless a bug in that SimpleAsyncTaskExecutor doesn't respect that due to isThrottleActive() returning false in that case. Fixed for 5.0 RC4 now, and to be backported to 4.3.11.

          For your purposes, setting the concurrency limit to 1 is indeed the right solution.

          Show
          juergen.hoeller Juergen Hoeller added a comment - NO_CONCURRENCY - as originally defined in ConcurrencyThrottleSupport - actually means no entrance to the monitor at all, so in this case no async execution at all. In that sense, 0 is the right value for it... but there is nevertheless a bug in that SimpleAsyncTaskExecutor doesn't respect that due to isThrottleActive() returning false in that case. Fixed for 5.0 RC4 now, and to be backported to 4.3.11. For your purposes, setting the concurrency limit to 1 is indeed the right solution.
          Hide
          simonwg Simon Wong added a comment -

          Other than just change the value for NO_CONCURRENCY from 0 to 1, will it be better to introduce a new constant,say, NO_ACCESS with value 0, to respect the original semantics (i.e. not entrance at all)? In that case, I could set the concurrency value at runtime (e.g. with JMX) to disable the access of the operation and it will throw some kinds of exception if access it.

          Show
          simonwg Simon Wong added a comment - Other than just change the value for NO_CONCURRENCY from 0 to 1, will it be better to introduce a new constant,say, NO_ACCESS with value 0, to respect the original semantics (i.e. not entrance at all)? In that case, I could set the concurrency value at runtime (e.g. with JMX) to disable the access of the operation and it will throw some kinds of exception if access it.
          Hide
          juergen.hoeller Juergen Hoeller added a comment -

          To be clear, the value of the constant hasn't changed here, and I do not intend to change it. The semantics of the existing constants are established for other ConcurrencyThrottleSupport usage scenarios, so the only thing we can really do is to tighten the definition there for the no-access case that it actually represents. From the perspective of the submitting thread, this doesn't seem so wrong either: There won't be any concurrent execution (no async tasks) next to the original thread.

          For your scenario, a custom concurrencyLimit value of 1 seems quite expressive, so I'm not sure there needs to be a dedicated constant for this in the first place. After all, the constants are just named placeholders for use in Java code... but the real values are the underlying integers anyway. For a JMX scenario, the values sound plausible enough as they are now: simply using -1 for unbounded, 0 for no access at all, and 1 for a maximum of one async task execution at the same time.

          So the actual fix here is just that NO_CONCURRENCY / 0 actually works now since isThrottleActive() returns true for it. Instead of just letting every task through like in unbounded mode, SimpleAsyncTaskExecutor will reject any task in no-concurrency now mode, as it was originally meant to do.

          Show
          juergen.hoeller Juergen Hoeller added a comment - To be clear, the value of the constant hasn't changed here, and I do not intend to change it. The semantics of the existing constants are established for other ConcurrencyThrottleSupport usage scenarios, so the only thing we can really do is to tighten the definition there for the no-access case that it actually represents. From the perspective of the submitting thread, this doesn't seem so wrong either: There won't be any concurrent execution (no async tasks) next to the original thread. For your scenario, a custom concurrencyLimit value of 1 seems quite expressive, so I'm not sure there needs to be a dedicated constant for this in the first place. After all, the constants are just named placeholders for use in Java code... but the real values are the underlying integers anyway. For a JMX scenario, the values sound plausible enough as they are now: simply using -1 for unbounded, 0 for no access at all, and 1 for a maximum of one async task execution at the same time. So the actual fix here is just that NO_CONCURRENCY / 0 actually works now since isThrottleActive() returns true for it. Instead of just letting every task through like in unbounded mode, SimpleAsyncTaskExecutor will reject any task in no-concurrency now mode, as it was originally meant to do.

            People

            • Assignee:
              juergen.hoeller Juergen Hoeller
              Reporter:
              simonwg Simon Wong
              Last updater:
              St├ęphane Nicoll
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Days since last comment:
                3 weeks, 3 days ago