Allow to make future chain request with ResponseFuture #419

rockerhieu · 2021-01-08T21:18:17Z

Closes #413

This PR allows to make future chain request with ResponseFuture:

  @override
  ResponseFuture<R> interceptUnary<Q, R>(ClientMethod<Q, R> method, Q request,
      CallOptions options, ClientUnaryInvoker<Q, R> invoker) {
    _invocations.add(InterceptorInvocation(_id, ++_unary, _streaming));
    return ResponseFuture.wrap(
            Future.delayed(Duration(seconds: 1), () => 'dummy'))
        .then((_) => invoker(method, request, _inject(options)))
        .then((foo) async => foo)
        .whenComplete(() => 'complete')
        .then((bar) => bar)
        .catchError((e, s) => print('$e at $s'))
        .timeout(Duration(seconds: 5));
  }

linux-foundation-easycla · 2021-01-08T21:18:21Z

The committers are authorized under a signed CLA.

✅ Hieu Rocker (e3d9c04)

rockerhieu · 2021-01-14T17:42:32Z

@mraleph hi, do you have a chance to take a look at this PR?

mraleph

Some initial review comments.

Thank you for the contribution!

mraleph · 2021-01-11T08:40:29Z

lib/src/client/common.dart

 class ResponseFuture<R> extends DelegatingFuture<R>
    with _ResponseMixin<dynamic, R> {
-  final ClientCall<dynamic, R> _call;
+  final ClientCall _call;


Why did you remove <dynamic, R>? This makes code less type safe.

Because of this: #419 (comment)

lib/src/client/common.dart

mraleph · 2021-01-24T08:03:20Z

lib/src/client/common.dart

+      ResponseFuture._clone(future, clientCall: _getClientCall(future, _call));
+
+  /// clientCall maybe be lost when converting from Future to ResponseFuture
+  static ResponseFuture<T> wrap<T>(Future<T> future, {ClientCall clientCall}) {


Do we actually have any use for wrap beyond testing?

We need a way to convert Future to ResponseFuture and vice versa. You can see we use it in the very first step in this example:

@override ResponseFuture<R> interceptUnary<Q, R>(ClientMethod<Q, R> method, Q request, CallOptions options, ClientUnaryInvoker<Q, R> invoker) { _invocations.add(InterceptorInvocation(_id, ++_unary, _streaming)); return ResponseFuture.wrap( Future.delayed(Duration(seconds: 1), () => 'dummy')) .then((_) => invoker(method, request, _inject(options))) .then((foo) async => foo) .whenComplete(() => 'complete') .then((bar) => bar) .catchError((e, s) => print('$e at $s')) .timeout(Duration(seconds: 5)); }

One idea- could you make an extension on Future / ResponseFuture instead?

lib/src/client/common.dart

mraleph · 2021-01-24T08:06:31Z

lib/src/client/common.dart


 abstract class _ResponseMixin<Q, R> implements Response {
-  ClientCall<Q, R> get _call;
+  ClientCall get _call;


We should not loose type safety.

.then(...) allows to transform a ResponseFuture<R> to a ResponseFuture<S> which will lose type safety. Do you have any suggestion?

@override ResponseFuture<S> then<S>(FutureOr<S> Function(R p1) onValue, {Function onError});

I realized that we don't actually use the type of call for anything. So I suggest the following change: it is indeed fine to drop types here, but the we should add types in default constructor to keep initial type safety:

ResponseFuture(ClientCall<dynamic, R> call) : _call = call, super(call.response .fold<R?>(null, _ensureOnlyOneResponse) .then(_ensureOneResponse));

mraleph · 2021-01-24T08:07:26Z

lib/src/client/common.dart


  @override
-  Future<void> cancel() => _call.cancel();
+  Future<void> cancel() => _call?.cancel();


I think _call becoming nullable is very unfortunate. Can we avoid that? I think it will make NNBD migration harder.

As Future doesn't have _call, converting a Future to a ResponseFuture will make _call == null. Let me revisit the change once more time to find a better approach.

Another thing we can do is having a convention that the final ResponseFuture in the chain must have a valid _call.

rockerhieu · 2021-01-24T23:19:58Z

Btw, as interceptUnary returns a ResponseFuture there is no way to mark interceptUnary as async as await always returns a Future.

Closes grpc#413

rockerhieu · 2021-06-03T17:28:47Z

@mraleph I know that this PR is quite outdated with recent changes about NNBD support. But what do you think about making _call optional and more generic?

mraleph · 2021-06-04T09:52:05Z

I think original gRPC library design is really gnarly here, it's kinda painting us into the corner.

For example, now that I look at it with fresh-eyes I realise that in your example:

  @override
  ResponseFuture<R> interceptUnary<Q, R>(ClientMethod<Q, R> method, Q request,
      CallOptions options, ClientUnaryInvoker<Q, R> invoker) {
    _invocations.add(InterceptorInvocation(_id, ++_unary, _streaming));
    return ResponseFuture.wrap(
            Future.delayed(Duration(seconds: 1), () => 'dummy'))
        .then((_) => invoker(method, request, _inject(options)))
        .then((foo) async => foo)
        .whenComplete(() => 'complete')
        .then((bar) => bar)
        .catchError((e, s) => print('$e at $s'))
        .timeout(Duration(seconds: 5));
  }

interceptor simply returns a ResponseFuture with a null call and the whole chaining exercise is kinda for nothing, nobody is going to see that call anyway (and other Response APIs are going to be "broken" by such interceptor).

So I guess the question here is: do we want to find a way to support asynchronous actions before calling invoker? I think that is the hard part, so my answer would be no.

In that case call is guaranteed to never be null, and we can remove public wrap. Basically the only way to get ReponseFuture is to call invoker or to call a method on ResponseFuture.

grpc#419

amondnet · 2021-06-17T09:23:38Z

@rockerhieu @mraleph Hi,

future.then() can returns Future<R> instead of Future<T>.

Future<R> then<R>(FutureOr<R> onValue(T value), {Function? onError});

Should ResponseFuture 's then also return ResponseFuture<R>? Is it possible to change the type of ClientCall? Or should I block type conversion?

final response = stub.sayHello(request).then( (HelloReply reply) => 'success');

Wouldn't it be better to create a new method ?

ResponseFuture<T> responseThen<T>(FutureOr<T> onValue(T value), {Function? onError});

stub.sayHello(request)
    .responseThen( (HelloReply reply) => reply);

mraleph · 2021-06-17T10:31:09Z

Is it possible to change the type of ClientCall? Or should I block type conversion?

The type of the call does not really matter. See my comment above: #419 (comment)

I think the biggest question for this PR is how to correctly support asynchronous operations before invoker is called and make sure that returned future gets access to ClientCall when that is available.

kengu · 2021-06-23T08:50:09Z

This PR enables features that are not practical to do another way, like transforming general grpc errors to typed exceptions. One workaround could be to use stream transformations, but it seems a bit excessive.

esenmx · 2021-09-02T07:01:32Z

The response type shouldn't be changed over interceptor, it's obviously very bad idea, on top of that we lose type-safety.

Interceptors should behave as complementary like refreshing tokens, then retrying the failed request, which requires an async function body.

Please just let wrap extension method with type-safety that's exactly what all we need.
If you need to change response type over interceptor, please consider using a sum type with oneof keyword.

rockerhieu · 2021-09-14T22:51:55Z

The response type shouldn't be changed over interceptor, it's obviously very bad idea, on top of that we lose type-safety.

Interceptors should behave as complementary like refreshing tokens, then retrying the failed request, which requires an async function body.

Please just let wrap extension method with type-safety that's exactly what all we need.
If you need to change response type over interceptor, please consider using a sum type with oneof keyword.

@esenmx can you elaborate on the wrap extension approach? Let's take one simple use case like measuring the time it takes for invoker(method, request, _inject(options)) to finish in interceptUnary. No response type changed.

esenmx · 2021-09-15T07:12:54Z

The response type shouldn't be changed over interceptor, it's obviously very bad idea, on top of that we lose type-safety.
Interceptors should behave as complementary like refreshing tokens, then retrying the failed request, which requires an async function body.
Please just let wrap extension method with type-safety that's exactly what all we need.
If you need to change response type over interceptor, please consider using a sum type with oneof keyword.

@esenmx can you elaborate on the wrap extension approach? Let's take one simple use case like measuring the time it takes for invoker(method, request, _inject(options)) to finish in interceptUnary. No response type changed.

If you are talking about merging the timeout value or any other metric info into your response data, you should use trailers instead. It's why made for, no type changed.

@override
ResponseFuture<R> interceptUnary<Q, R>(
    ClientMethod<Q, R> method, Q request, CallOptions options, ClientUnaryInvoker<Q, R> invoker) {
  final stopwatch = Stopwatch()..start();
  return invoker(method, request, options)
    ..trailers.then((trailers) {
      trailers['timeout'] = stopwatch.elapsedMilliseconds.toString();
    });
}

// request
final result = client.doSomething();
result.trailers.then((value) {
  print('timeout: ${value['timeout']}');
});

mraleph · 2022-05-02T08:58:13Z

I think instead of adding support for chaining ResponseFuture objects we could allow specifying response interceptors separate from the call interceptors. See draft PR here: #548

Let me know if that would solve all of your potential use cases.

mraleph · 2022-05-04T12:32:17Z

I am going to close this PR because direct chaining has some hidden pitfalls (e.g. one highlighted in #419 (comment)).

pull-request-size bot added the size/M label Jan 8, 2021

rockerhieu force-pushed the master branch 3 times, most recently from d485aff to be63dfe Compare January 8, 2021 21:25

rockerhieu mentioned this pull request Jan 8, 2021

Interceptor for handling response #413

Open

mraleph requested changes Jan 24, 2021

View reviewed changes

rockerhieu force-pushed the master branch 4 times, most recently from ce5b881 to 74b55f9 Compare January 24, 2021 22:46

Allow to make future chain request with ResponseFuture

706c07e

Closes grpc#413

rockerhieu force-pushed the master branch from 74b55f9 to 706c07e Compare March 8, 2021 05:21

amondnet added a commit to amondnet/grpc-dart that referenced this pull request Jun 17, 2021

feat: allow to make future chain request with ResponseFuture

58b166e

grpc#419

savy-91 mentioned this pull request Mar 21, 2022

feat: allow to make future chain request with ResponseFuture and ResponseStream #543

Closed

mraleph closed this May 4, 2022

Allow to make future chain request with ResponseFuture #419

Allow to make future chain request with ResponseFuture #419

Uh oh!

Conversation

rockerhieu commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rockerhieu commented Jan 14, 2021

Uh oh!

mraleph left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rockerhieu Jan 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rockerhieu commented Jan 24, 2021

Uh oh!

rockerhieu commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mraleph commented Jun 4, 2021

Uh oh!

amondnet commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mraleph commented Jun 17, 2021

Uh oh!

kengu commented Jun 23, 2021

Uh oh!

esenmx commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rockerhieu commented Sep 14, 2021

Uh oh!

esenmx commented Sep 15, 2021

Uh oh!

mraleph commented May 2, 2022

Uh oh!

mraleph commented May 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rockerhieu commented Jan 8, 2021 •

edited

Loading

linux-foundation-easycla bot commented Jan 8, 2021 •

edited

Loading

rockerhieu Jan 24, 2021 •

edited

Loading

rockerhieu commented Jun 3, 2021 •

edited

Loading

amondnet commented Jun 17, 2021 •

edited

Loading

esenmx commented Sep 2, 2021 •

edited

Loading