Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
First implementation of gRPC based on SwiftNIO #281
First implementation of gRPC based on SwiftNIO #281
Changes from 22 commits
68e0a09
d891c38
0866ea0
6374d88
ae35416
29ad4f6
7b592f1
d4c6c0e
488d243
137a1c1
de5ec1c
495cdf4
c4efbf7
367bd32
4915ce5
0ec7137
973618b
ddef365
036ea30
0aa0af0
a66d449
b3bbc7d
c7a9eef
7a131a6
b3537b9
5503cb6
a347855
0a879a9
7ab7a05
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like we need this based on how it's used below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm keeping this around to ensure the context (and its promises) do not get deallocated prematurely. Removed the
FIXME
, though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be able to remove these from all 4 handlers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm keeping this around to ensure the context (and its promises) do not get deallocated prematurely. Removed the FIXME, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice if we could remove this state and instead rely on
self.eventObserver
being nil-ed out when it's no longer needed. Then we could potentially simplyassert(self.eventObserver == nil)
since it's a library error if this happens and removehasReceivedRequest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that we can use
eventObserver
for this (but I don't think we shouldassert
there, as clients could be violating the gRPC protocol by sending multiple messages); please take another look.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this a little clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced
complete that framework
withcomplete that future
, sorry for the mistake.