Skip to content

Server handler interceptor state machine #1394

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

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Apr 28, 2022

Motivation:

The existing async server handler has a state machine which deals with
the interceptor pipeline and the lifecycle of user handler. These are
really two separate entities each with its own state machine.

This PR is the first in a series which will eventually update the async
server handler to make use of the two state machines.

Modifications:

  • Add a 'ServerInterceptorStateMachine' and tests. This acts as a filter
    for inbound and outbound request and response messages, both as they
    enter and exit the interceptor pipeline. Message parts delievered in the
    wrong order lead to the RPC being cancelled, parts delievered after
    the RPC has finished lead to them being dropped. Cancelling the RPC
    informs the caller to nil out the associated interceptor pipeline.
  • Tests
  • This is not used anywhere (yet).

Result:

Interceptor state machine is in place.

@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Apr 28, 2022
Motivation:

The existing async server handler has a state machine which deals with
the interceptor pipeline and the lifecycle of user handler. These are
really two separate entities each with its own state machine.

This PR is the first in a series which will eventually update the async
server handler to make use of the two state machines.

Modifications:

- Add a 'ServerInterceptorStateMachine' and tests. This acts as a filter
  for inbound and outbound request and response messages, both as they
  enter and exit the interceptor pipeline. Message parts delievered in the
  wrong order lead to the RPC being cancelled, parts delievered after
  the RPC has finished lead to them being dropped. Cancelling the RPC
  informs the caller to nil out the associated interceptor pipeline.
- Tests
- This is not used anywhere (yet).

Result:

Interceptor state machine is in place.
@glbrntt glbrntt force-pushed the gb-async-handler-interceptor-state-machine branch from 07eaf9c to 814de4b Compare April 29, 2022 09:32
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Nice one, LGTM

Copy link
Collaborator

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

Nice work! A more general question, does the separation of the state machines bring us any closer to having Interceptors on a Channel level or maybe does it make it harder?

@glbrntt
Copy link
Collaborator Author

glbrntt commented May 3, 2022

Nice work! A more general question, does the separation of the state machines bring us any closer to having Interceptors on a Channel level or maybe does it make it harder?

No, it has no bearing on that change.

@glbrntt glbrntt merged commit 49c3af7 into grpc:1.7.1-async-await May 3, 2022
@glbrntt glbrntt deleted the gb-async-handler-interceptor-state-machine branch May 3, 2022 10:05
glbrntt added a commit to glbrntt/grpc-swift that referenced this pull request May 3, 2022
Motivation:

See grpc#1394.

Modifications:

- Add a 'ServerHandlerStateMachine' and tests.
- This is not used anywhere (yet).

Result:

Handler state machine is in place.
glbrntt added a commit that referenced this pull request May 4, 2022
Motivation:

See #1394.

Modifications:

- Add a 'ServerHandlerStateMachine' and tests.
- This is not used anywhere (yet).

Result:

Handler state machine is in place.
glbrntt added a commit to glbrntt/grpc-swift that referenced this pull request May 11, 2022
Motivation:

In grpc#1394 and grpc#1396 we introduced new state machines for the server
interceptors and handler. This change updates the async server handler
to make use of them.

Modifications:

- Add the relevant `@inlinable` and `@usableFromInline` annotations to
  both state machines.
- Refactor the async server handler to use both state machines.
- Refactor async handler tests to use a 'real' event loop; the previous
  mix of embedded and async was unsafe.
- Re-enable TSAN

Result:

- Better separation between interceptors and user func
- TSAN is happier
- Resolves grpc#1362
glbrntt added a commit to glbrntt/grpc-swift that referenced this pull request May 11, 2022
Motivation:

In grpc#1394 and grpc#1396 we introduced new state machines for the server
interceptors and handler. This change updates the async server handler
to make use of them.

Modifications:

- Add the relevant `@inlinable` and `@usableFromInline` annotations to
  both state machines.
- Refactor the async server handler to use both state machines.
- Refactor async handler tests to use a 'real' event loop; the previous
  mix of embedded and async was unsafe.
- Re-enable TSAN

Result:

- Better separation between interceptors and user func
- TSAN is happier
- Resolves grpc#1362
glbrntt added a commit that referenced this pull request May 16, 2022
Motivation:

In #1394 and #1396 we introduced new state machines for the server
interceptors and handler. This change updates the async server handler
to make use of them.

Modifications:

- Add the relevant `@inlinable` and `@usableFromInline` annotations to
  both state machines.
- Refactor the async server handler to use both state machines.
- Refactor async handler tests to use a 'real' event loop; the previous
  mix of embedded and async was unsafe.
- Re-enable TSAN

Result:

- Better separation between interceptors and user func
- TSAN is happier
- Resolves #1362
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants