Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Could Mono.block(Duration timeout) throw java.util.concurrent.TimeoutException? #2267

Closed
Mahoney opened this issue Jul 14, 2020 · 4 comments
Closed
Labels
status/declined We feel we shouldn't currently apply this change/suggestion

Comments

@Mahoney
Copy link

Mahoney commented Jul 14, 2020

Mono.block(Duration timeout) currently throws IllegalStateException if the timeout expires. This seems odd as a) it isn't an illegal state, it's a perfectly possible state that we're explicitly anticipating and b) the more correct java.util.concurrent.TimeoutException is thrown in similar circumstances when Mono.timeout(Duration timeout).subscribe() times out.

The code that throws the IllegalStateException is:

throw new IllegalStateException("Timeout on blocking read for " + timeout + " " + unit);

@reactorbot reactorbot added the ❓need-triage This issue needs triage, hasn't been looked at by a team member yet label Jul 14, 2020
@simonbasle simonbasle added status/need-decision This needs a decision from the team and removed ❓need-triage This issue needs triage, hasn't been looked at by a team member yet labels Jul 15, 2020
@simonbasle
Copy link
Member

TimeoutException is a checked exception. It is not a problem on the timeout operator because it is not thrown but propagated through onError, but on block it would force us to add it to the signature of the method (and all current callers to declare a catch block).

@simonbasle
Copy link
Member

Note that

Mono.never()
    .timeout(Duration.ofSeconds(1))
    .block();

Throws a Runtime(Reactive)Exception, with the TimeoutException as the cause.
Maybe we could align on that for block(Duration)?

This would at least bring consistency in how to deal with timeout exceptions in blocking code:

try {
    longMono.block();
}
catch (SpecificRuntimeException sre) {
    //do something specific
}
catch (RuntimeException e) {
    if (Exceptions.unwrap(e) instanceof TimeoutException) {
        //deal with timeout
    }
    else {
        //generic exception
    }
}

@Mahoney
Copy link
Author

Mahoney commented Jul 15, 2020

Working in Kotlin, so I'd forgotten about the checked / unchecked problem. Though I guess it's a breaking change anyway as people might have code expecting an IllegalStateException for a timeout now...

I don't really have a clear solution, just wanted to raise it as something that confused me as a reactor newbie.

@simonbasle simonbasle added status/declined We feel we shouldn't currently apply this change/suggestion and removed status/need-decision This needs a decision from the team labels Jul 15, 2020
@simonbasle
Copy link
Member

I'm inclined to leave it as is. The breaking change (or the breaking behavior that I suggested above) are IMO not worth it, especially considering that blocking is for corner cases only and is generally strongly discouraged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/declined We feel we shouldn't currently apply this change/suggestion
Projects
None yet
Development

No branches or pull requests

3 participants