linkedin / parseq

Asynchronous Java made easier
Apache License 2.0
1.17k stars 266 forks source link

runAndWaitException doesn't wait for plan to complete #278

Closed hiteshsharma closed 4 years ago

hiteshsharma commented 4 years ago

In our codebase we explicitly verify completion of side-effect tasks using assertTrue(sideEffectTask.isDone()). One of our tests runs a Task where there are 2 parallel tasks running (let's call them task A and B) such that taskA succeeds and has a sideEffect S attached to it while Task B fails. In this case, even though B failed, A and S are expected to complete but in our tests the assertion for completion of S sometimes fails. I was able to replicate this by writing the following quick test in ParseqUnitTestHelperTest class and I see that the reason for this is that runAndWaitException uses runAndWait rather than runAndWaitForPlanToComplete which waits for side-effect tasks to complete.

  @Test
  public void testRunAndWaitException() throws Exception {
    ParSeqUnitTestHelper parSeqUnitTestHelper = new ParSeqUnitTestHelper();
    parSeqUnitTestHelper.setUp();
    Task<String> delayedSideEffect = parSeqUnitTestHelper.delayedValue("delayed", 1, TimeUnit.SECONDS);
    parSeqUnitTestHelper.runAndWaitException(
      Task.par(
        Task.value("a").withSideEffect(a -> delayedSideEffect),
        parSeqUnitTestHelper.delayedFailure(new Exception(), 100, TimeUnit.MILLISECONDS)
      ),
      Exception.class
    );
    assertTrue(delayedSideEffect.isDone());
    parSeqUnitTestHelper.tearDown();
  }

The fix for this would be to either use runAndWaitForPlanToComplete instead of runAndWait in runAndWaitException (verified that it works) or provide a new runAndWaitForPlanToCompleteException which uses runAndWaitForPlanToComplete underneath but operates like runAndWaitException

I can fix this myself but just want to confirm which one does the parseq team prefer.

mchen07 commented 4 years ago

a new API will be better

hiteshsharma commented 4 years ago

@mchen07 please review and merge when you get a chance. Thanks!!

evanw555 commented 4 years ago

Looks like this was merged, so I'm closing this now. Please re-open if I'm mistaken. Thanks for contributing