Skip to content

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Sep 16, 2024

Motivation:

The interceptors API has a context which, at the moment, only includes
the name of the RPC. This information is generally useful so should be
propagated to the server handler too. Information on the context can
also be extended later to include things like the identity of the remote
peer, or info about the state of the RPC.

Modifications:

  • Rename 'ServerInterceptorContext' to 'ServerContext'
  • Make the transport the source of the context and have that provide it
    to the 'listen' method. Propagate this through the server stack to the
    generated stubs.
  • Update code generator to include the context
  • Update generated code
  • Update services

Results:

RPC handlers have a context provided by a transport

Motivation:

The interceptors API has a context which, at the moment, only includes
the name of the RPC. This information is generally useful so should be
propagated to the server handler too. Information on the context can
also be extended later to include things like the identity of the remote
peer, or info about the state of the RPC.

Modifications:

- Rename 'ServerInterceptorContext' to 'ServerContext'
- Make the transport the source of the context and have that provide it
  to the 'listen' method. Propagate this through the server stack to the
  generated stubs.
- Update code generator to include the context
- Update generated code
- Update services

Results:

RPC handlers have a context provided by a transport
@glbrntt
Copy link
Collaborator Author

glbrntt commented Sep 16, 2024

Split into two commits:

  • 359c6c3 contains the bulk of the changes
  • 0e8b637 is the result of re-running the code generator

@glbrntt glbrntt marked this pull request as ready for review September 16, 2024 16:16
@glbrntt glbrntt requested a review from gjcairo September 16, 2024 16:16
@glbrntt glbrntt added the version/v2 Relates to v2 label Sep 16, 2024
}

group.addTask {
try await server.listen { stream in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aren't we getting a warning because we're not using context here? Should we _ it, or should we assert the contained method descriptor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, no warning for not using it. I don't mind whether we change it to _ or leave it as context. Let me know what you prefer.

I'll add a separate test checking we get the descriptor in the context.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, that's fine, I'm okay with the name. I thought warnings were emitted in this case :D


try await withThrowingTaskGroup(of: Void.self) { group in
group.addTask {
try await transport.listen { stream in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aren't we getting a warning because we're not using context here? Should we _ it, or should we assert the contained method descriptor?


try transport.acceptStream(firstStream)

try await transport.listen { stream in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aren't we getting a warning because we're not using context here? Should we _ it, or should we assert the contained method descriptor?

) async throws {
try await withThrowingTaskGroup(of: Void.self) { group in
group.addTask {
try await self.serverTransport.listen { stream in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aren't we getting a warning because we're not using context here?

@glbrntt glbrntt merged commit 7789f1e into grpc:main Sep 19, 2024
11 of 14 checks passed
@glbrntt glbrntt deleted the v2/propagate-server-context branch September 19, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

version/v2 Relates to v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants