Synchronize read & write of `TaskGroupWithContext`'s `err` variable#37

Merged

alitto merged 1 commit intoalitto:masterfrom

thekondor:1.8.1-fix-group-wait-race

Oct 14, 2022

Contributor

thekondor commented Oct 14, 2022

Context

That is not stated whether Wait() & Submit() for TaskGroupWithContext are allowed for usage from different execution threads. As end user I expected that it is.

Problem

// thread 1
group.Submit(func() error {
   return errors.New("failed")
})

// thread 2
group.Wait()

leads to race condition because of non-synchronized (write & read) operations to .err.

The problem is detectable in 100% cases by a built-in race checker.

PR

The change introduces usage of mutex to synchronize access to this variable at the above mentioned code flows.


          fix: synchronize Group's err

7a514ef

alitto approved these changes

View reviewed changes

Owner

alitto left a comment

Thanks a lot for this contribution @thekondor! 🙌. I'll merge this and release a patch version to include it.
Very nice PR description 🙂

alitto merged commit fbc6f5d into alitto:master

estahn pushed a commit to estahn/k8s-image-swapper that referenced this pull request


          chore(release): 1.3.2 [skip ci]

23a9535

## [1.3.2](v1.3.1...v1.3.2) (2022-11-01)

### ⬆️ Dependencies

* **deps:** Bump actions/cache from 3.0.10 to 3.0.11 ([#365](#365)) ([4e88994](4e88994)), closes [actions/cache#946](actions/cache#946) [actions/cache#950](actions/cache#950) [actions/cache#956](actions/cache#956) [actions/cache#950](actions/cache#950) [#956](#956) [#950](#950) [#946](#946)
* **deps:** bump actions/cache from 3.0.8 to 3.0.10 ([#358](#358)) ([921d9e2](921d9e2)), closes [#809](#809) [#833](#833) [#810](#810) [#931](#931) [#942](#942) [#930](#930) [#920](#920) [#936](#936) [#925](#925)
* **deps:** Bump actions/setup-python from 4.2.0 to 4.3.0 ([#362](#362)) ([05a7ab3](05a7ab3)), closes [#517](#517) [#499](#499) [#443](#443) [#477](#477) [#479](#479) [#491](#491) [#492](#492) [#517](#517) [#503](#503) [#499](#499) [#495](#495) [#443](#443) [#492](#492) [#491](#491)
* **deps:** Bump docker/login-action from 2.0.0 to 2.1.0 ([#364](#364)) ([ca6a535](ca6a535)), closes [#275](#275) [#252](#252) [#292](#292) [#298](#298) [#299](#299) [#299](#299) [#298](#298) [#292](#292) [#275](#275)
* **deps:** Bump github.com/alitto/pond from 1.8.1 to 1.8.2 ([#371](#371)) ([417edd5](417edd5)), closes [alitto/pond#37](alitto/pond#37) [#37](#37)
* **deps:** bump github.com/aws/aws-sdk-go from 1.44.100 to 1.44.109 ([#359](#359)) ([b9acd7d](b9acd7d)), closes [#4574](https://github.com/estahn/k8s-image-swapper/issues/4574) [#4573](https://github.com/estahn/k8s-image-swapper/issues/4573) [#4571](https://github.com/estahn/k8s-image-swapper/issues/4571) [#4568](https://github.com/estahn/k8s-image-swapper/issues/4568) [#4567](https://github.com/estahn/k8s-image-swapper/issues/4567) [#4566](https://github.com/estahn/k8s-image-swapper/issues/4566) [#4565](https://github.com/estahn/k8s-image-swapper/issues/4565) [#4562](https://github.com/estahn/k8s-image-swapper/issues/4562) [#4561](https://github.com/estahn/k8s-image-swapper/issues/4561)
* **deps:** Bump github.com/aws/aws-sdk-go from 1.44.109 to 1.44.114 ([#363](#363)) ([832a21d](832a21d)), closes [#4580](https://github.com/estahn/k8s-image-swapper/issues/4580) [#4579](https://github.com/estahn/k8s-image-swapper/issues/4579) [#4578](https://github.com/estahn/k8s-image-swapper/issues/4578) [#4576](https://github.com/estahn/k8s-image-swapper/issues/4576) [#4575](https://github.com/estahn/k8s-image-swapper/issues/4575)
* **deps:** Bump github.com/aws/aws-sdk-go from 1.44.114 to 1.44.121 ([#377](#377)) ([26f4103](26f4103)), closes [#4596](https://github.com/estahn/k8s-image-swapper/issues/4596) [#4595](https://github.com/estahn/k8s-image-swapper/issues/4595) [#4593](https://github.com/estahn/k8s-image-swapper/issues/4593) [#4519](https://github.com/estahn/k8s-image-swapper/issues/4519) [#4590](https://github.com/estahn/k8s-image-swapper/issues/4590) [#4589](https://github.com/estahn/k8s-image-swapper/issues/4589) [#4587](https://github.com/estahn/k8s-image-swapper/issues/4587) [#4586](https://github.com/estahn/k8s-image-swapper/issues/4586)
* **deps:** Bump github.com/aws/aws-sdk-go from 1.44.121 to 1.44.126 ([#383](#383)) ([b7e43d9](b7e43d9)), closes [#4603](https://github.com/estahn/k8s-image-swapper/issues/4603) [#4602](https://github.com/estahn/k8s-image-swapper/issues/4602) [#4601](https://github.com/estahn/k8s-image-swapper/issues/4601) [#4600](https://github.com/estahn/k8s-image-swapper/issues/4600) [#4598](https://github.com/estahn/k8s-image-swapper/issues/4598)
* **deps:** bump github.com/containers/image/v5 from 5.22.0 to 5.23.0 ([#360](#360)) ([250d9e4](250d9e4)), closes [#1665](https://github.com/estahn/k8s-image-swapper/issues/1665) [#1666](https://github.com/estahn/k8s-image-swapper/issues/1666) [#1664](https://github.com/estahn/k8s-image-swapper/issues/1664) [#1662](https://github.com/estahn/k8s-image-swapper/issues/1662)
* **deps:** Bump github.com/dgraph-io/ristretto from 0.1.0 to 0.1.1 ([#368](#368)) ([0e9c9df](0e9c9df)), closes [#285](#285) [dgraph-io/ristretto#311](dgraph-io/ristretto#311) [#304](#304) [dgraph-io/ristretto#304](dgraph-io/ristretto#304) [#287](#287) [dgraph-io/ristretto#307](dgraph-io/ristretto#307) [#285](#285) [dgraph-io/ristretto#311](dgraph-io/ristretto#311) [#304](#304) [dgraph-io/ristretto#304](dgraph-io/ristretto#304) [#287](#287) [dgraph-io/ristretto#307](dgraph-io/ristretto#307) [#312](#312) [#285](#285) [#310](#310) [#306](#306) [#309](#309) [#308](#308) [#287](#287) [#307](#307) [#304](#304)
* **deps:** Bump github.com/go-co-op/gocron from 1.17.0 to 1.17.1 ([#380](#380)) ([8c4cef8](8c4cef8)), closes [go-co-op/gocron#382](go-co-op/gocron#382) [go-co-op/gocron#386](go-co-op/gocron#386) [go-co-op/gocron#386](go-co-op/gocron#386) [#386](#386) [#382](#382)
* **deps:** Bump github.com/gruntwork-io/terratest from 0.40.22 to 0.40.23 ([#367](#367)) ([a07149b](a07149b)), closes [gruntwork-io/terratest#1186](gruntwork-io/terratest#1186) [gruntwork-io/terratest#1189](gruntwork-io/terratest#1189) [#1189](https://github.com/estahn/k8s-image-swapper/issues/1189) [#1186](https://github.com/estahn/k8s-image-swapper/issues/1186)
* **deps:** Bump github.com/gruntwork-io/terratest from 0.40.23 to 0.40.24 ([#378](#378)) ([e57939d](e57939d)), closes [gruntwork-io/terratest#1191](gruntwork-io/terratest#1191) [#1191](https://github.com/estahn/k8s-image-swapper/issues/1191)
* **deps:** Bump github.com/slok/kubewebhook/v2 from 2.3.0 to 2.5.0 ([#372](#372)) ([249a996](249a996)), closes [#218](#218) [#217](#217) [#187](#187)
* **deps:** Bump github.com/spf13/cobra from 1.5.0 to 1.6.0 ([#373](#373)) ([39cfd45](39cfd45)), closes [#1003](#1003) [#1802](https://github.com/estahn/k8s-image-swapper/issues/1802) [#1760](https://github.com/estahn/k8s-image-swapper/issues/1760) [#1707](https://github.com/estahn/k8s-image-swapper/issues/1707) [#1813](https://github.com/estahn/k8s-image-swapper/issues/1813) [#1788](https://github.com/estahn/k8s-image-swapper/issues/1788) [#1621](https://github.com/estahn/k8s-image-swapper/issues/1621) [#1467](https://github.com/estahn/k8s-image-swapper/issues/1467) [#1643](https://github.com/estahn/k8s-image-swapper/issues/1643) [#1643](https://github.com/estahn/k8s-image-swapper/issues/1643) [#1762](https://github.com/estahn/k8s-image-swapper/issues/1762) [#1771](https://github.com/estahn/k8s-image-swapper/issues/1771) [#1776](https://github.com/estahn/k8s-image-swapper/issues/1776) [#1766](https://github.com/estahn/k8s-image-swapper/issues/1766) [#1782](https://github.com/estahn/k8s-image-swapper/issues/1782) [#1803](https://github.com/estahn/k8s-image-swapper/issues/1803) [#1783](https://github.com/estahn/k8s-image-swapper/issues/1783) [#1387](https://github.com/estahn/k8s-image-swapper/issues/1387) [#1792](https://github.com/estahn/k8s-image-swapper/issues/1792) [#1744](https://github.com/estahn/k8s-image-swapper/issues/1744) [#1748](https://github.com/estahn/k8s-image-swapper/issues/1748) [#1726](https://github.com/estahn/k8s-image-swapper/issues/1726) [#1656](https://github.com/estahn/k8s-image-swapper/issues/1656) [#1779](https://github.com/estahn/k8s-image-swapper/issues/1779) [#1741](https://github.com/estahn/k8s-image-swapper/issues/1741) [#1742](https://github.com/estahn/k8s-image-swapper/issues/1742) [#1745](https://github.com/estahn/k8s-image-swapper/issues/1745) [#1759](https://github.com/estahn/k8s-image-swapper/issues/1759) [#1772](https://github.com/estahn/k8s-image-swapper/issues/1772) [#1819](https://github.com/estahn/k8s-image-swapper/issues/1819) [#1800](https://github.com/estahn/k8s-image-swapper/issues/1800) [#1809](https://github.com/estahn/k8s-image-swapper/issues/1809) [#1804](https://github.com/estahn/k8s-image-swapper/issues/1804) [#1467](https://github.com/estahn/k8s-image-swapper/issues/1467) [#1003](#1003) [#1813](https://github.com/estahn/k8s-image-swapper/issues/1813) [#1621](https://github.com/estahn/k8s-image-swapper/issues/1621) [#1792](https://github.com/estahn/k8s-image-swapper/issues/1792) [#1788](https://github.com/estahn/k8s-image-swapper/issues/1788) [#1815](https://github.com/estahn/k8s-image-swapper/issues/1815) [#1819](https://github.com/estahn/k8s-image-swapper/issues/1819) [#1707](https://github.com/estahn/k8s-image-swapper/issues/1707) [#1760](https://github.com/estahn/k8s-image-swapper/issues/1760)
* **deps:** Bump github.com/spf13/cobra from 1.6.0 to 1.6.1 ([#384](#384)) ([ffe3ef6](ffe3ef6)), closes [#1839](https://github.com/estahn/k8s-image-swapper/issues/1839) [#1841](https://github.com/estahn/k8s-image-swapper/issues/1841)
* **deps:** Bump github.com/stretchr/testify from 1.8.0 to 1.8.1 ([#379](#379)) ([a0e1429](a0e1429)), closes [#1283](https://github.com/estahn/k8s-image-swapper/issues/1283)
* **deps:** Bump golangci/golangci-lint-action from 3.2.0 to 3.3.0 ([#375](#375)) ([abcf765](abcf765)), closes [#586](#586) [#584](#584) [#582](#582) [#580](#580) [#578](#578) [#576](#576) [#577](#577) [#575](#575)
* **deps:** Bump k8s.io/api from 0.25.1 to 0.25.3 ([#366](#366)) ([c233527](c233527)), closes [#112808](https://github.com/estahn/k8s-image-swapper/issues/112808) [cheftako/automated-cherry-pick-of-#112689](https://github.com/cheftako/automated-cherry-pick-of-/issues/112689)

NOMORECOFFEE reviewed

View reviewed changes

group.go

		@@ -86,5 +93,9 @@ func (g *TaskGroupWithContext) Wait() error {
		case <-g.ctx.Done():
		}

NOMORECOFFEE Nov 1, 2022

You have to return g.ctx.Err(), otherwise we will get a data race.

ctx, cancel := context.WithTimeout(ctx, time.Second)
defer cancel()

g, gctx := pool.GroupContext(ctx)
g.Submit(func() error { select { case <-gctx.Done(): return gctx.Err() }})
err := g.Wait() // err == nil

Owner

alitto Jan 26, 2023

Oh, good catch @NOMORECOFFEE!. If you don't mind, please open a PR with suggested changes and I can review it later. Otherwise I can send one. Thank you! 🙌

Owner

alitto Jan 31, 2023

I just released v1.8.3 with a fix for this, please check it out and let me know if you find anything. Thank you!

alitto mentioned this pull request

Return Context error if it is canceled before at least 1 task failed #40

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet