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

Delay creating event observers for client streaming calls #523

Merged
merged 2 commits into from
Jul 25, 2019

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Jul 23, 2019

Motivation:

Providers for client and bidirectional streaming calls require
the user provide a future stream-event handler to handle requests
from the client. However, these methods get called as the pipeline
handling an incoming call is being configured, as these methods
also expose promises for response (for client streaming) and
call status (for bidirectional streaming) it is possible for these
to be fulfilled before the pipeline has been configured. Since
no handler is in place to deal with the promised types the server
will fatal error as the first handler in place will fail to unwrap
the promised type.

Modifications:

Delay the creation of event observers for client and
bidirectional streaming calls until their handlers have been added
to the pipeline.

Result:

Client streaming and bidirectional streaming calls can fulfill
their response and status promises outside of their stream handlers.

@glbrntt
Copy link
Collaborator Author

glbrntt commented Jul 23, 2019

Resolves #520

@glbrntt glbrntt force-pushed the immediately-failing-provider branch 2 times, most recently from 0296e3e to f3f645e Compare July 24, 2019 13:27
@MrMage
Copy link
Collaborator

MrMage commented Jul 24, 2019

Do you think we could set the observer factories to nil once they are called (or if we determine that we won't need to call them at all), as a precaution against retain cycles?

Motivation:

Providers for client and bidirectional streaming calls require
the user provide a future stream-event handler to handle requests
from the client. However, these methods get called as the pipeline
handling an incoming call is being configured, as these methods
also expose promises for response (for client streaming) and
call status (for bidirectional streaming) it is possible for these
to be fulfilled before the pipeline has been configured. Since
no handler is in place to deal with the promised types the server
will fatal error as the first handler in place will fail to unwrap
the promised type.

Modifications:

Delay the creation of event observers for client and
bidirectional streaming calls until their handlers have been added
to the pipeline.

Result:

Client streaming and bidirectional streaming calls can fulfill
their response and status promises outside of their stream handlers.
@glbrntt
Copy link
Collaborator Author

glbrntt commented Jul 24, 2019

Sure, that's a good idea!

@glbrntt glbrntt force-pushed the immediately-failing-provider branch from f3f645e to 5fc8ba1 Compare July 24, 2019 15:00
@MrMage MrMage merged commit d4bcd92 into grpc:nio Jul 25, 2019
@glbrntt glbrntt deleted the immediately-failing-provider branch August 5, 2020 09:06
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