Skip to content

Fix race between stream registration and use #7886

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 4, 2023
Merged

Fix race between stream registration and use #7886

merged 2 commits into from
Jan 4, 2023

Conversation

dmcgowan
Copy link
Member

@dmcgowan dmcgowan commented Dec 31, 2022 •
edited
Loading

Prevent a race where a client may attempt to use a stream before it is registered.

Addresses #7885

@dmcgowan
[streaming] move response packet after registration
d7ef6cb 
Prevent a race where a client may attempt to use a stream
before it is registered.

Signed-off-by: Derek McGowan <derek@mcg.dev>
@k8s-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@dmcgowan
Add lease to transfer to preserve streams during transfer
a5b9796 
Signed-off-by: Derek McGowan <derek@mcg.dev>
@dmcgowan dmcgowan marked this pull request as ready for review January 4, 2023 07:05
@dmcgowan
Copy link
Member Author

dmcgowan commented Jan 4, 2023

I'm not seeing the error show up after many runs of CI with both the fixes in place.

@dmcgowan dmcgowan changed the title [streaming] Move response packet after registration Fix race between stream registration and use Jan 4, 2023
estesp
estesp approved these changes Jan 4, 2023
View reviewed changes
Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 72e5ddb into containerd:main Jan 4, 2023
@fangn2
Copy link
Contributor

fangn2 commented Jan 5, 2023

I'm not seeing the error show up after many runs of CI with both the fixes in place.

@dmcgowan
Side question, how can I trigger multiple builds on the same commit, or rerun a failed build as a contributor? Seems that's the option only available to admins.
With random failures on tests, there seems no good way to retry a build except a force push(by updating the commit).

@samuelkarp
Copy link
Member

Side question, how can I trigger multiple builds on the same commit, or rerun a failed build as a contributor? Seems that's the option only available to admins.

Unfortunately that's a limitation of GitHub that we're not able to do much about. Force-push, close/reopen, or pinging a maintainer and asking one of us to do it are the ways to trigger a rerun. (If you see a failure you think is unrelated, it's worth noting that in a comment on the PR too.)

@fangn2
Copy link
Contributor

fangn2 commented Jan 5, 2023

@samuelkarp Thanks for clarifying that!

@fangn2 fangn2 mentioned this pull request Jan 6, 2023
Closed
@dmcgowan dmcgowan deleted the fix-transfer-register-ordering branch April 20, 2024 00:45
kiashok added a commit to kiashok/containerd that referenced this pull request Oct 23, 2024
@kiashok
Merged PR 7482211: Update containerd fork-external/main with upstream…
d66a29a 
… main

Updates ADO containerd fork-external/main with upstream containerd main @commit hash: f9f8455
containerd@f9f8455

Additional changes in the PR:
- update linux container image version
- remove copying LICENSE file in shared-build-stages.yml
- additions to .gdnsuppress

Related work items: containerd#5674, containerd#7393, containerd#7661, containerd#7685, containerd#7810, containerd#7850, containerd#7861, containerd#7879, containerd#7880, containerd#7881, containerd#7882, containerd#7883, containerd#7886, containerd#7887, containerd#7888, containerd#7891, containerd#7892, containerd#7893, containerd#7894, containerd#7903, containerd#7904, containerd#7905, containerd#7906, containerd#7907, containerd#7908, containerd#7911, containerd#7913, containerd#7914, containerd#7917, containerd#7925, containerd#7927, containerd#7928, containerd#7929, containerd#7932, containerd#7935, containerd#7943, containerd#7946, containerd#7948, containerd#7957, containerd#7958, containerd#7960, containerd#7963, containerd#7969, containerd#7970, containerd#7973
kiashok added a commit to kiashok/containerd that referenced this pull request Oct 23, 2024
@kiashok
Merged PR 7534707: Update fork-external/main with upstream containerd…
f02f374 
…/main

Update fork-external/main with upstream containerd/containerd/main at commit hash 3d32da8

Related work items: containerd#5674, containerd#7129, containerd#7393, containerd#7661, containerd#7685, containerd#7810, containerd#7850, containerd#7861, containerd#7882, containerd#7883, containerd#7886, containerd#7891, containerd#7892, containerd#7893, containerd#7903, containerd#7904, containerd#7905, containerd#7906, containerd#7907, containerd#7908, containerd#7911, containerd#7913, containerd#7914, containerd#7917, containerd#7925, containerd#7927, containerd#7928, containerd#7929, containerd#7932, containerd#7935, containerd#7943, containerd#7946, containerd#7948, containerd#7957, containerd#7958, containerd#7959, containerd#7960, containerd#7963, containerd#7968, containerd#7969, containerd#7970, containerd#7973, containerd#7985, containerd#7987, containerd#7994, containerd#8005
@gopilola gopilola mentioned this pull request Mar 27, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxpv mxpv mxpv approved these changes

@estesp estesp estesp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Pull Request Review
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dmcgowan @k8s-ci-robot @fangn2 @samuelkarp @mxpv @estesp