Skip to content

Fixes retrials without Thread.sleep when thread gets interrupted#627

Merged
velo merged 2 commits into
OpenFeign:masterfrom
MichalSzewczyk:master
Apr 8, 2018
Merged

Fixes retrials without Thread.sleep when thread gets interrupted#627
velo merged 2 commits into
OpenFeign:masterfrom
MichalSzewczyk:master

Conversation

@MichalSzewczyk

@MichalSzewczyk MichalSzewczyk commented Jan 19, 2018

Copy link
Copy Markdown
Contributor

This PR aims to avoid flood of retries when thread, in which retryer is running, gets interrupted. Default retryer will rethrow FeignException in case of interrupted state.
Fixes #624
Fixes #643

@velo

velo commented Apr 5, 2018

Copy link
Copy Markdown
Member

This makes lots of sense to me, I just wonder if we can have a test somehow.

@rage-shadowman

rage-shadowman commented Apr 5, 2018

Copy link
Copy Markdown
Contributor

Might I suggest a test:

  @Test
  public void failsOnInterruptedException() {
    Default retryer = new Retryer.Default();

    Thread.currentThread().interrupt();
    RetryableException expected = new RetryableException(null, null, new Date(System.currentTimeMillis() + 5000));
    try {
      retryer.continueOrPropagate(expected);
      Thread.interrupted(); // reset interrupted flag in case it wasn't
      Assert.fail("Retryer continued despite interruption");
    } catch (RetryableException e) {
      Assert.assertTrue("Interrupted status not reset", Thread.interrupted());
      Assert.assertEquals("Retry attempt not registered as expected", 2, retryer.attempt);
      Assert.assertEquals("Unexpected exception found", expected, e);
    }
  }

@MichalSzewczyk

Copy link
Copy Markdown
Contributor Author

new Date(5000) could be replaced with null.
Btw is it safe to interrupt current thread? Thread.currentThread().interrupt(); code will set flag back to interrupted and as a consequence all tests run after this one will have interrupted flag set to true.

@MichalSzewczyk

Copy link
Copy Markdown
Contributor Author

What do you think about running retryer in other thread?

  @Test
  public void defaultRetryerShouldPropagateFeignExceptionWhenThreadGetsInterrupted() {
    Retryer retryer = new Retryer.Default();
    Thread thread = new Thread(() -> assertThatThrowsException(RetryableException.class, () -> retryer.continueOrPropagate(new RetryableException(null, null))));
    thread.start();
    thread.interrupt();
  }

@rage-shadowman

rage-shadowman commented Apr 5, 2018

Copy link
Copy Markdown
Contributor

You're right. Since the Retryer code resets the interrupted flag, you need to check it immediately afterward again. My code missed that part.

Just add (in the catch block):

Assert.assertTrue(Thread.interrupted());

[EDIT: I updated the code block with some additional assertions and an interrupted flag reset]

@rage-shadowman

rage-shadowman commented Apr 5, 2018

Copy link
Copy Markdown
Contributor

PS - I think that if you run it in another thread, you need to check that thread for assertion states. It's probably easier to just do it in the current thread and just Assert.assertTrue(Thread.interrupted())

The test should fail if you revert your src/main changes (and only this test should fail) and it should pass once the changes are re-applied.

assertThatThrowsExceptionAndThreadIsInterrupted(RetryableException.class,
() -> retryer.continueOrPropagate(new RetryableException(null, null))));
thread.start();
thread.interrupt();

@rage-shadowman rage-shadowman Apr 7, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can this test ever fail with no assertions while swallowing all exceptions in another thread?

@MichalSzewczyk

Copy link
Copy Markdown
Contributor Author

Ok, I added test suggested by you.

@velo velo merged commit ea51e70 into OpenFeign:master Apr 8, 2018
@OpenFeign OpenFeign deleted a comment from MichalSzewczyk Apr 12, 2018
velo pushed a commit that referenced this pull request Oct 7, 2024
* Fixes retrials without Thread.sleep when thread gets interrupted

* Added test to verify if default retryer propagates RetryableException exception when thread is interrupted
velo pushed a commit that referenced this pull request Oct 8, 2024
* Fixes retrials without Thread.sleep when thread gets interrupted

* Added test to verify if default retryer propagates RetryableException exception when thread is interrupted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants