ARTEMIS-450 Deadlocked queues over AMQP after too many cancellations

[clebertsuconic]

No description provided.

[franz1981]

nice!!!!! 👍

[franz1981]

I've built up this test:

   @Test
   public void testVerifyInitialDelayChanged() {
      final long initialDelay = 10;
      final long period = 100;
      final ActiveMQScheduledComponent local = new ActiveMQScheduledComponent(scheduledExecutorService, executorService, initialDelay, period, TimeUnit.MILLISECONDS, false) {
         @Override
         public void run() {

         }
      };
      local.start();
      final long newInitialDelay = 1000;
      //the parameters are valid?
      assert initialDelay != newInitialDelay && newInitialDelay != period;
      local.setInitialDelay(newInitialDelay);
      local.stop();
      Assert.assertEquals("the initial dalay can't change", newInitialDelay, local.getInitialDelay());
   }

And it fail: maybe is better to create a start(long forceInitialDelay) method and call it into restartIfNeeded passing it period, wdyt?

[clebertsuconic]

Nope. Initial delay should be at the beginnnjng only. Why would you want a further initial delay when it’s not initial any longer. Just to keep it simple.

[franz1981]

@clebertsuconic But if (at the end of the test) I will start the component, it will have a changed configuration...it can lead to subtle bugs...

[franz1981]

@clebertsuconic it is "initial" on each start after a stop (eg how i use it for the JDBC Node manager)

[clebertsuconic]

What do you need an initialDelay different from the frequency anyways? What is the usecase?

[franz1981]

@clebertsuconic When I have a very big period but I need the first call to happen right after the start of the component. IMO a user expectation is that the component's start will honour the configuration provided if the component was in a stopped state...

[clebertsuconic]

@franz1981 I'm using an initialDelay = -1 by default now, and added some code around on start.

[franz1981]

I'm receiving an error in the test testVerifyDefaultInitialDelay:

java.lang.AssertionError: The initial delay must be defaulted to the period 
Expected :100
Actual   :-1

Modifying things like this (and leaving the constructor as it is) doesn't break any tests:

   // this will restart the scheduled component upon changes
   private void restartIfNeeded() {
      if (isStarted()) {
         stop();
         //do not need to start with the initialDelay: the component was already running
         start(this.period);
      }
   }

   private void start(final long initialDelay) {
      if (future != null) {
         // already started
         return;
      }

      if (scheduledExecutorService == null) {
         scheduledExecutorService = new ScheduledThreadPoolExecutor(1, getThreadFactory());
         startedOwnScheduler = true;

      }

      if (onDemand) {
         return;
      }

      this.millisecondsPeriod = timeUnit.convert(period, TimeUnit.MILLISECONDS);

      if (period >= 0) {
         future = scheduledExecutorService.scheduleWithFixedDelay(runForScheduler, initialDelay, period, timeUnit);
      } else {
         logger.tracef("did not start scheduled executor on %s because period was configured as %d", this, period);
      }
   }

   @Override
   public synchronized void start() {
      start(this.initialDelay);
   }

[clebertsuconic]

your test wasn’t validating semantics. The change is good.

[franz1981]

@clebertsuconic It is a get of field after a set of the same that is not getting the configured/expected value: it is not what a user could expect.

[franz1981]

@clebertsuconic It is not following the principle of least astonishment from the API (and doc) perspective: if you remove the getInitialDelayprobably you could use safetly that change, but considering that all the other properties are exposed, probably it is not the right choice.
That's my 2 cents on it: I'm happy anyway that the functionality isn't broken (for my usage I mean) 👍

[clebertsuconic]

@franz1981 It's just that I changed the meaning of getInitialDelay... -1 meaning unset.. and use the previous semantic... the change you did broke some usage.. so I had to have a null state for initialDelay where I would get the previous semantic... that's all

[clebertsuconic]

using your own words.. the least astonishment way would be to have the initialDelay nullable (or -1 on this case).... I'm fixing what was broken before.

[franz1981]

@clebertsuconic I do not agree with that: the change I did was validated with tests and the documentation says that when the initial delay isn't specified is defaulted to period, so the expectation on it is that it will be defaulted to period in a getInitialDelay too.
As I said, for me it's the same but IMO if you want to add a new semantic over a null initialDelay you need to specify it in some way (ie docs/tests and/or even better, on the API) in order to avoid subtle bugs and/or to avoid to change tests that are only checking a get after a set.

[clebertsuconic]

@franz1981 nope.. you broke a test Franz.

[clebertsuconic]

@franz1981 lets not waste time.. this is such a small component... you added a new parameter that was clashing with previous usage.. all I'm doing is fix it accordingly to the previous version after the tests you broke.