-
Notifications
You must be signed in to change notification settings - Fork 79
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
feat: support initial-response in RPC based event streams #1165
Merged
Merged
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
9d850f3
add initial-response support
dayaffe b9c1505
use feature branch in smithy-swift for CI
dayaffe 3670303
Merge branch 'main' into day/initial-messages
dayaffe cc532f7
fix swiftlint
dayaffe 673ef0e
Merge branch 'main' into day/initial-messages
dayaffe 028f495
remove local test code
dayaffe 6400ec9
add unit tests for initialResponse
dayaffe 3d665fe
Merge branch 'main' into day/initial-messages
dayaffe 0c304c7
Merge branch 'main' into day/initial-messages
dayaffe 7d2d2d1
Merge branch 'main' into day/initial-response-rpc-support
dayaffe a27a254
revert unnecessary change to Package.swift
dayaffe 31b678d
remove extra space in Package.swift
dayaffe 0bc0773
Update Package.swift
dayaffe d2c7f86
add comments + make method private
dayaffe d6c52bf
add additional part of test for logic codegen into Clients
dayaffe 7a3aa9c
Merge branch 'main' into day/initial-response-rpc-support
dayaffe e12aa66
ensure onInitialResponseReceived continuation is called only once by …
dayaffe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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 is required, in all cases, that the continuation gets called (it's a rather large memory leak and a frozen path of execution if it doesn't.)
Considering that
retrieveInitialResponse()
will in some cases save the continuation to an instance var, how do we guarantee that it gets called in every case?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 believe we can guarantee that it is called in every case.
If
didProcessInitialMessage
istrue
the continuation is called immediately through thecompletion
callback:completion(initialMessage)
If
didProcessInitialMessage
isfalse
the continuation is stored in theonInitialResponseReceived
instance variable:self.onInitialResponseReceived = completion
onInitialResponseReceived
must be invoked. It gets invoked during theonComplete
phase ofEventStreamMessageDecoder
which should always be invoked at least once per event.onComplete
in either case whether we spot aninitial-response
event-type message, either we setself.onInitialResponseReceived
toself.initialMessage
or tonil
. That should cover all cases.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 in separate conversation that immediately after running the continuation stored in
didProcessInitialMessage
, set it to nil so it is released and it cannot be called again.