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

Retain cause / message when Promise.fail is throwing IllegalStateException #5419

Open
alzimmermsft opened this issue Dec 13, 2024 · 2 comments

Comments

@alzimmermsft
Copy link

alzimmermsft commented Dec 13, 2024

Describe the feature

When Promise.fail(Throwable) or Promise.fail(String) are called with an already completed Promise the reason for why those methods are called can be lost. Using Promise.tryFail(Throwable) and Promise.tryFail(String) can prevent the exception from being thrown by Promise but if the fail methods are being used they should at least include the cause for the exception being thrown.

Ex:

Change

default void fail(Throwable cause) {
  if (!tryFail(cause)) {
    throw new IllegalStateException("Result is already complete");
  }
}

to

default void fail(Throwable cause) {
  if (!tryFail(cause)) {
    throw new IllegalStateException("Result is already complete", cause);
  }
}

This will at least retain the cause information if the exception thrown isn't handled appropriately.

Use cases

Loss of information when an error occurs.

Contribution

I'd be happy to contribute if this change is deemed appropriate.

@vietj
Copy link
Member

vietj commented Dec 13, 2024

I am not sure using it as cause is the right thing to do, since both are not related directly, the cause on the illegal state exception could be misleading in logs.

Instead something like a PromiseAlreadyCompletedException extending IllegalStateException could used with an optional object that is the completion (which is the result or a failure). Users might want to know about the same with complete(T) and wonder which object triggered a success completion.

@alzimmermsft
Copy link
Author

That makes sense to me :)

Was mostly looking to see if there was an improvement that could be done in this area for code, which I'm basing on my code where I had a corner case where this exception was thrown, where information wasn't lost. I was able to root cause and fix my code to handle this more gracefully, but the reason for why this issue triggered I still don't know as there wasn't a way to grab the first Promise.cause due to the structure of my code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants