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

Provide a channel handler using the client state machine #601

Merged
merged 3 commits into from
Oct 25, 2019

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Oct 1, 2019

Motivation:

8db70dd (#580) introduced a client state machine which enables us to
reduce the number of channel handlers the client has and confine much of
the stream state tracking to a single place. However, none of our
channel handlers were designed to use it!

Modifications:

Create a client channel handler using the aforementioned state machine
and update the state machine to talk in HTTP/2 concepts to avoid having
to use an additional handler to translate between HTTP/1 and HTTP/2
concepts.

Result:

Functionally, nothing, as the channel handler is not yet used.

Motivation:

8db70dd (grpc#580) introduced a client state machine which enables us to
reduce the number of channel handlers the client has and confine much of
the stream state tracking to a single place. However, none of our
channel handlers were designed to use it!

Modifications:

Create a client channel handler using the aforementioned state machine
and update the state machine to talk in HTTP/2 concepts to avoid having
to use an additional handler to translate between HTTP/1 and HTTP/2
concepts.

Result:

Functionally, nothing, as the channel handler is not yet used.
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 1, 2019

CLA Check
The committers are authorized under a signed CLA.

Copy link
Collaborator

@MrMage MrMage left a comment

Choose a reason for hiding this comment

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

Looks very good overall; no objections to the logic. My biggest concern would be about dropping gRPC-Web support; not sure if we can/should simply leave transcoding to an out-of-process proxy dedicated to that.

Sources/GRPC/GRPCClientChannelHandler.swift Show resolved Hide resolved
Sources/GRPC/GRPCClientChannelHandler.swift Outdated Show resolved Hide resolved
Sources/GRPC/GRPCClientChannelHandler.swift Show resolved Hide resolved
Sources/GRPC/GRPCClientChannelHandler.swift Outdated Show resolved Hide resolved
Sources/GRPC/GRPCClientChannelHandler.swift Show resolved Hide resolved
Sources/GRPC/GRPCClientChannelHandler.swift Outdated Show resolved Hide resolved
Sources/GRPC/GRPCClientChannelHandler.swift Show resolved Hide resolved
Sources/GRPC/GRPCClientChannelHandler.swift Outdated Show resolved Hide resolved
Sources/GRPC/GRPCClientChannelHandler.swift Show resolved Hide resolved
Sources/GRPC/GRPCClientStateMachine.swift Show resolved Hide resolved
@glbrntt
Copy link
Collaborator Author

glbrntt commented Oct 25, 2019

@MrMage are you okay with these changes / can I merge?

@MrMage
Copy link
Collaborator

MrMage commented Oct 25, 2019

@MrMage are you okay with these changes / can I merge?

Yes, sorry!

@glbrntt glbrntt merged commit 9add2b5 into grpc:nio Oct 25, 2019
@glbrntt glbrntt deleted the gb-channel-handler branch October 25, 2019 09:26
@glbrntt
Copy link
Collaborator Author

glbrntt commented Oct 25, 2019

@MrMage are you okay with these changes / can I merge?

Yes, sorry!

No problem :)

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

Successfully merging this pull request may close these issues.

2 participants