grpc-spring icon indicating copy to clipboard operation
grpc-spring copied to clipboard

fix: run dynamic call credentials via executor

Open ST-DDT opened this issue 1 year ago • 3 comments

Fixes #958

  • #958

ST-DDT avatar Nov 25 '23 14:11 ST-DDT

Caveat: Better to offload the credential fetching when it's non-cached one. Assuming most of the request can reuse a cached the credential, this may pay a small cost for most of the rpcs.

fengli79 avatar Nov 27 '23 19:11 fengli79

So would it be better to revert the change and just add a hint to the javadocs?

Or maybe introduce a new variant that it is explicitly called "async" that takes a (executor) -> CompletableFuture<String>? That way users can return CompletableFuture.completedFuture() if nothing async needs to be done.

private final Function<Executor, CompletableFuture<String>> extraHeadersFutureSupplier;

@Override
        public void applyRequestMetadata(final RequestInfo requestInfo, final Executor appExecutor,
                final MetadataApplier applier) {

            extraHeadersFutureSupplier.apply(executor).whenComplete((header, e) -> {
                if (e == null) {
                    final Metadata extraHeaders = new Metadata();
                    extraHeaders.put(AUTHORIZATION_HEADER, header);
                    applier.apply( extraHeaders);
                } else {
                    applier.fail(Status.UNAUTHENTICATED.withCause(e));
                }
            });
        }

On the other hand we could just let the user implement that temselves.

ST-DDT avatar Nov 28 '23 12:11 ST-DDT

Yes, I feel the dynamic credential doesn't provide too much value to the end users. This is an important enough detail (cached/non-cached/async/blocking) which should be exposed to the implementer of the credential. Either way works, but I personally like the ideal to just let the user implement that themselves.

fengli79 avatar Nov 29 '23 01:11 fengli79