Skip to content

Add server interceptor acting as a middleware #762

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ThHareau
Copy link

@ThHareau ThHareau commented Mar 3, 2025

Fix for #591

Example of usage:

class SentryInstrumenterInterceptor extends $grpc.ServerInterceptor {
  @override
  Stream<R> interceptStreaming<Q, R>(
      $grpc.ServiceCall call,
      $grpc.ServiceMethod<Q, R> method,
      Stream<Q> requests,
      $grpc.ServerStreamingInvoker<Q, R> invoker) async* {
    final name = method.name;
    final transaction = Sentry.startTransaction(method.name, 'grpc');
    print('Before $name');

    yield* super.interceptStreaming(call, method, requests, invoker);

    print('After $name');
    transaction.finish();
  }
}

I did not check if this PR solves #611, anyway if it does not it would probably be nice to open a second PR, this one is complex enough

Copy link

linux-foundation-easycla bot commented Mar 3, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

Comment on lines +40 to +41
Stream<R> intercept<Q, R>(ServiceCall call, ServiceMethod<Q, R> method,
Stream<Q> requests, ServerStreamingInvoker<Q, R> invoker) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal here is to be close to the Client implementation. As the handler is a Stream, I did not add unary. It is probably doable tho.

@ThHareau ThHareau marked this pull request as ready for review March 3, 2025 18:14
Copy link

github-actions bot commented Mar 6, 2025

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

Coverage ⚠️
File Coverage
lib/grpc.dart 💔 Not covered
lib/src/server/handler.dart 💚 96 %
lib/src/server/interceptor.dart 💚 100 %
lib/src/server/server.dart 💚 82 % ⬆️ 0 %
lib/src/server/service.dart 💚 97 % ⬆️ 1 %

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

API leaks ⚠️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbols
grpc Any
$1.Duration

This check can be disabled by tagging the PR with skip-leaking-check.

@ThHareau
Copy link
Author

ThHareau commented Mar 6, 2025

Should I do anything about the changelog health check?

@mosuem
Copy link
Contributor

mosuem commented Mar 6, 2025

Should I do anything about the changelog health check?

No, the breaking change should not be regarded as breaking and is from a different PR (note to self: the check should show that the breakage was introduced elsewhere..). The version is probably fine.

I'll take a closer look at this once I get to it, quite busy ATM unfortunately. But thanks a lot already for the contribution!

@ThHareau
Copy link
Author

ThHareau commented Mar 6, 2025

Should I do anything about the changelog health check?

No, the breaking change should not be regarded as breaking and is from a different PR (note to self: the check should show that the breakage was introduced elsewhere..). The version is probably fine.

I'll take a closer look at this once I get to it, quite busy ATM unfortunately. But thanks a lot already for the contribution!

No worries, it can wait a bit 🙂 Just fyi, I'll be away from the 12th to end of march, so I won't be able to answer during this period. But I'll reply afterward.

@ThHareau ThHareau force-pushed the master branch 2 times, most recently from a317691 to 064818b Compare March 31, 2025 09:29
@ThHareau ThHareau requested a review from mosuem March 31, 2025 09:30
@ThHareau
Copy link
Author

ThHareau commented Apr 7, 2025

CI should pass now :)

@ThHareau
Copy link
Author

ThHareau commented Apr 7, 2025

@mosuem Is the changelog failure something I can act on?

Copy link
Contributor

@mosuem mosuem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - forgot to press the submit review button...

@mosuem
Copy link
Contributor

mosuem commented Apr 15, 2025

error: pubspec version (4.0.4) and changelog (4.1.0) don't agree

@ThHareau
Copy link
Author

It should be fixed @mosuem

@ThHareau
Copy link
Author

@mosuem any idea on how to make the CI pass? I see that both checks failed with error Error: Process completed with exit code 143., which corresponds to a SIGTERM if I understood correctly. Could you try relaunching it?

@mosuem
Copy link
Contributor

mosuem commented Apr 16, 2025

Locally, the tests hang as well, which can be helped by commenting out serverInterceptors: serverInterceptors..insert(0, serverInterceptor), - which seems to be the culprit.

@ThHareau
Copy link
Author

ThHareau commented Apr 16, 2025

My bad @mosuem , the issue was in the inlinement of invoker and delegate. invoker is apparently reassigned in the same scope as the callback execution, creating an infinite loop.

I've rollbacked my changes and added a comment explaining the reason.

Small repro of the issue if you wanna try in a dartpad
invoker() {
  print('Invoked');
}

final wrappers = <Function(Function())>[
  (cb) {
    print('before function');
    cb();
    print('after function');
  },
];

void main() {
  var flow = invoker;

  for (final interceptor in wrappers.reversed) {
    flow = () => interceptor(flow);
  }

  flow();
}

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

Successfully merging this pull request may close these issues.

None yet

2 participants