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

improve exception handling in CompletionStage* strategies #202

Open
Ladicek opened this issue Mar 4, 2020 · 1 comment
Open

improve exception handling in CompletionStage* strategies #202

Ladicek opened this issue Mar 4, 2020 · 1 comment

Comments

@Ladicek
Copy link
Contributor

Ladicek commented Mar 4, 2020

All CompletionStage* strategies currently have duplicated exception handling, both for synchronous case and asynchronous case. They typically look similar to:

try {
    CompletableFuture<V> result = new CompletableFuture<>();
    delegate.apply(ctx).whenComplete((value, exception) -> {
        if (exception == null) {
            result.complete(value);
        } else {
            result.completeExceptionally(exception); // <1>
        }
    });
    return result;
} catch (Exception e) {
    return failedStage(e); // <2>
}

This is just the core, all the strategies do metric updates, internal state updates, strategy-specific logic, etc. This all has to be duplicated between <1> and <2>.

It would be much better if we could just rely on asynchronous exception handling. The only place exceptions can be thrown synchronously from is (or should be) the ultimate end-user method call (i.e., the Invocation strategy). We can wrap it into another strategy that will make sure exceptions are not propagated synchronously. Implementation of such strategy is actually the exact code given above (but we'd use CompletionStages.propagateCompletion instead). All the other CompletionStage* strategies then don't have to care about synchronous exceptions and can drop the try/catch structure.

@Ladicek Ladicek mentioned this issue Mar 4, 2020
@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 3, 2020

At this point, I'm not exactly sure we should do this. The synchronous exception handling at least guards against mistakes in our own code -- which are really easy to make, because exceptions can be thrown from anywhere. I'll keep this issue open for now, because the duplication isn't nice and there might be something else we could do.

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

No branches or pull requests

1 participant