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

Tidy up some of the logic in the server call contexts #1018

Merged
merged 1 commit into from
Oct 27, 2020

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Oct 26, 2020

Motivation:

The server call contexts rely on the service provider completing a
promise, either for the response or status depending on the streaming
type. Doing so then writes the relevant parts into the pipeline.
However, we also have to deal with the promises being failed, and since
the logic there is a little circuitous the callback chains were a little
hard to follow and had some redundancy.

Modifications:

  • Add a 'processError(_:delegate:)` to the base server call context to
    transform an error into a status and trailers.
  • Refactor the promise completion chains for the unary and streaming
    response call contexts.

Result:

  • Code is easier to follow. Less duplication.

Motivation:

The server call contexts rely on the service provider completing a
promise, either for the response or status depending on the streaming
type. Doing so then writes the relevant parts into the pipeline.
However, we also have to deal with the promises being failed, and since
the logic there is a little circuitous the callback chains were a little
hard to follow and had some redundancy.

Modifications:

- Add a 'processError(_:delegate:)` to the base server call context to
  transform an error into a status and trailers.
- Refactor the promise completion chains for the unary and streaming
  response call contexts.

Result:

- Code is easier to follow. Less duplication.
@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Oct 26, 2020
@glbrntt
Copy link
Collaborator Author

glbrntt commented Oct 26, 2020

Could you review this @Davidde94? (I can't tag you as a reviewer.)

Copy link
Contributor

@Davidde94 Davidde94 left a comment

Choose a reason for hiding this comment

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

I think I get it, looks good to me

@glbrntt glbrntt requested a review from Lukasa October 27, 2020 08:40
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, LGTM

@Lukasa Lukasa merged commit 6e9fe81 into grpc:main Oct 27, 2020
@glbrntt glbrntt deleted the gb-server-context-tidy branch October 27, 2020 09:47
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