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

streaming: fix snapshot cache bug #9772

Merged
merged 2 commits into from Feb 16, 2021
Merged

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Feb 16, 2021

Previously a snapshot created as part of a resume-stream request could have incorrectly cached the newSnapshotToFollow event. When the next subscribe request was received with Index=0 the returned snapshot would cause clients to error because they received an unexpected framing event.

Fixed the bug by saving a snapshot without the framing event, then creating a new snapshot to prepend the framing event.

when the cache entry is created from resuming a stream.
@dnephin dnephin added type/bug Feature does not function as expected theme/streaming Related to Streaming connections between server and client labels Feb 16, 2021
@hashicorp-ci
Copy link
Contributor

🤔 Double check that this PR does not require a changelog entry in the .changelog directory. Reference

Previously a snapshot created as part of a resumse-stream request could have incorrectly
cached the newSnapshotToFollow event. This would cause clients to error because they
received an unexpected framing event.
@dnephin dnephin force-pushed the streamin-fix-bad-cached-snapshot branch from be3c23f to ba3a1b9 Compare February 16, 2021 17:52
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging February 16, 2021 17:52 Inactive
@vercel vercel bot temporarily deployed to Preview – consul February 16, 2021 17:52 Inactive
Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Thanks LGTM!

Copy link
Member

@hanshasselberg hanshasselberg left a comment

Choose a reason for hiding this comment

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

LGTM

@dnephin dnephin merged commit 363d738 into master Feb 16, 2021
@dnephin dnephin deleted the streamin-fix-bad-cached-snapshot branch February 16, 2021 20:28
@hashicorp-ci
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/328108.

@hashicorp-ci
Copy link
Contributor

🍒✅ Cherry pick of commit 363d738 onto release/1.9.x succeeded!

hashicorp-ci pushed a commit that referenced this pull request Feb 16, 2021
dizzyup pushed a commit that referenced this pull request Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/streaming Related to Streaming connections between server and client type/bug Feature does not function as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants