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

BearerToken error handling fail #117

Open
hporchet opened this issue Nov 5, 2024 · 4 comments
Open

BearerToken error handling fail #117

hporchet opened this issue Nov 5, 2024 · 4 comments

Comments

@hporchet
Copy link

hporchet commented Nov 5, 2024

When the application of the metadata is in error, the fail method crash : apply() or fail() already called and hide the exception of the apply method.

public class BearerToken extends CallCredentials {
    ...

    @Override
    public void applyRequestMetadata(RequestInfo requestInfo, Executor executor, MetadataApplier applier) {
        executor.execute(() -> {
            try {
                Metadata headers = new Metadata();
                headers.put(META_DATA_KEY, header);
                applier.apply(headers); // crash
            } catch (Throwable e) {
                applier.fail(Status.UNAUTHENTICATED.withCause(e)); // can't be called after crashed, error : apply() or fail() already called
            }
        });
    }
}
@tstirrat15
Copy link
Contributor

@hporchet do you have a reproducible example? I'd like to write a test around this if possible.

It seems as though this could happen if there were nested/sequential withCallCredentials calls or some other piece of code that's making an apply call. Otherwise the apply seems to be just going and doing a hashmap merge, which I wouldn't expect to error in normal operation.

Where are you seeing the problem manifest?

@hporchet
Copy link
Author

I have seen this problem when i was on windows and the server return NOT_IMPLEMENTED to all grpc call. The request was correctly send but the not implemented raise a STATUS_EXCEPTION at the apply level.

@tstirrat15
Copy link
Contributor

Hmm... That makes sense. We probably shouldn't be catching all Throwables there either. Lemme see if I can narrow that sufficiently.

@hporchet
Copy link
Author

I think a simple log of the error before call fail() on debug mode seems enough, it just need to be easily find when a problem occur.

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

2 participants