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.

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

fix: avoid race condition by not running exec/start twice #702

Merged
merged 1 commit into from
May 25, 2021
Merged

fix: avoid race condition by not running exec/start twice #702

merged 1 commit into from
May 25, 2021

Conversation

ZauberNerd
Copy link
Contributor

ContainerExecAttach implicitly runs ContainerExecStart while attaching
to stdout/stderr.
Ref: https://github.com/moby/moby/blob/e02bc91dcbf6ab70ab39352f3577a4394b0f863a/client/container_exec.go#L40

Calling both can lead to a race condition as observed in #627

Fixes: #627

@ZauberNerd ZauberNerd requested a review from a team as a code owner May 23, 2021 15:43
@codecov
Copy link

codecov bot commented May 23, 2021 •
edited
Loading

Codecov Report

Merging #702 (22b45e3) into master (0f04942) will increase coverage by 1.22%.
The diff coverage is 59.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #702      +/-   ##
==========================================
+ Coverage   49.27%   50.49%   +1.22%     
==========================================
  Files          23       23              
  Lines        2401     2535     +134     
==========================================
+ Hits         1183     1280      +97     
- Misses       1090     1114      +24     
- Partials      128      141      +13
Impacted Files Coverage Δ
pkg/container/docker_run.go 1.85% <0.00%> (-0.08%) ⬇️
pkg/common/git.go 53.08% <30.15%> (-6.72%) ⬇️
pkg/model/workflow.go 30.71% <33.33%> (+5.00%) ⬆️
pkg/model/planner.go 34.56% <41.37%> (+1.48%) ⬆️
pkg/container/docker_pull.go 36.17% <64.70%> (+17.98%) ⬆️
pkg/runner/step_context.go 71.99% <71.42%> (+3.04%) ⬆️
pkg/runner/run_context.go 79.84% <95.23%> (+3.44%) ⬆️
pkg/runner/command.go 90.58% <100.00%> (+2.35%) ⬆️
pkg/runner/runner.go 76.92% <100.00%> (+0.45%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d794e2f...22b45e3. Read the comment docs.

catthehacker
catthehacker previously approved these changes May 23, 2021
View reviewed changes
@mergify mergify bot requested a review from a team May 23, 2021 17:37
cplee
cplee previously approved these changes May 23, 2021
View reviewed changes
@cplee
Copy link
Contributor

cplee commented May 24, 2021

@ZauberNerd - i need you to rebase this PR before it can be merged...or allow reviewers to update the branch.

@mergify
Copy link
Contributor

mergify bot commented May 24, 2021

@ZauberNerd this pull request has failed checks 🛠

@mergify mergify bot added the needs-work Extra attention is needed label May 24, 2021
@ZauberNerd @KnisterPeter
fix: avoid race condition by not running exec/start twice
22b45e3 
ContainerExecAttach implicitly runs ContainerExecStart while attaching
to stdout/stderr.
Ref: https://github.com/moby/moby/blob/e02bc91dcbf6ab70ab39352f3577a4394b0f863a/client/container_exec.go#L40

Calling both can lead to a race condition as observed in nektos#627

Fixes: nektos#627

Co-authored-by: Markus Wolf <[email protected]>
@ZauberNerd ZauberNerd dismissed stale reviews from cplee and catthehacker via 22b45e3 May 25, 2021 09:57
@mergify
Copy link
Contributor

mergify bot commented May 25, 2021 •
edited by catthehacker
Loading

@ZauberNerd this pull request has (not) failed checks 🛠

👀

@mergify mergify bot removed the needs-work Extra attention is needed label May 25, 2021
@mergify mergify bot requested a review from a team May 25, 2021 10:04
@ZauberNerd
Copy link
Contributor Author

@ZauberNerd - i need you to rebase this PR before it can be merged...or allow reviewers to update the branch.

I've rebased the branch. I can't see the "Allow maintainers to edit the PR" checkbox. Maybe it doesn't work for organisation accounts?

@mergify mergify bot merged commit 8a9167d into nektos:master May 25, 2021
@ZauberNerd ZauberNerd deleted the fix-exec-already-running branch May 25, 2021 16:10
@snyk-bot snyk-bot mentioned this pull request Jul 15, 2021
Open
@xNinjaKittyx xNinjaKittyx mentioned this pull request Apr 22, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@catthehacker catthehacker catthehacker approved these changes

@cplee cplee cplee approved these changes

Assignees
No one assigned
Labels
size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Issue: Race condition: "Exec command ... is already running"
3 participants
@ZauberNerd @cplee @catthehacker