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

Use new state machines in async server handler #1403

Merged
merged 2 commits into from May 16, 2022

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented May 11, 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:

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
// We don't distinguish between having sent the status or not; we just tell the interceptor
// state machine that we want to cancel. It will inform us whether to generate and send a
// status or not.
switch self.interceptorStateMachine.interceptResponseStatus() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not clear to me why we intercept the response status here. Can you elaborate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I ummed and ahhed about this. The rationale is that if we're mid-RPC then we should let the interceptors know the rpc is going away. The downside is that it requires the interceptors to play ball and not do anything async. Clearly I didn't do a good job of convincing myself because on L344 we just send out the status if we cancel.

@glbrntt glbrntt merged commit 5ece0b8 into grpc:1.7.1-async-await May 16, 2022
@glbrntt glbrntt deleted the gb-async-handler branch May 16, 2022 12:13
@glbrntt glbrntt added the semver-patch • Requires a SemVer Patch version change label Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch • Requires a SemVer Patch version change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants