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

fix: repository cloning race condition (#2341) #2348

Merged
merged 3 commits into from
Jul 27, 2022
Merged

fix: repository cloning race condition (#2341) #2348

merged 3 commits into from
Jul 27, 2022

Conversation

ribejara-te
Copy link
Contributor

This is my proposal to fix #2341.

@ribejara-te
Copy link
Contributor Author

The use of sync.Once was too cumbersome by the way parameters are passed down the stack.

@ribejara-te
Copy link
Contributor Author

Tests failed because(*sync.Mutex).TryLock() was added in go1.18.

-.-

@ribejara-te
Copy link
Contributor Author

I mean... upgrading to Go 1.18 doesn't seem to break anything, and it will happen at some point, right?

@ribejara-te
Copy link
Contributor Author

I've replaced go1.17 sync's Mutex with golang.org/x/sync/semaphore's Weighted.
It's technically a semaphore, but it works for us since we can check if it's acquired or not, unlike go.17 sync's Mutex.

We should switch back to sync (git revert 15ee16d7) once Atlantis is upgraded to Go 1.18.

@jamengual
Copy link
Contributor

we are planning to update to go1.18 soon

@ribejara-te
Copy link
Contributor Author

we are planning to update to go1.18 soon

How soon though? This is slowing our whole engineering dept down massively.

Plans that used to take 1-2min now take upwards for 40min.

@ribejara-te
Copy link
Contributor Author

I'm fine raising the PR switching back to sync for when Atlantis is upgraded to go1.18, but it'd be great to have this fixed ASAP.

@jamengual
Copy link
Contributor

jamengual commented Jun 30, 2022 via email

@ribejara-te
Copy link
Contributor Author

@jamengual so do I revert the switch to semaphores? do you also want me to update go.mod to go1.18?

@jamengual
Copy link
Contributor

let's wait a few days to see if @chenrui333 is successful at upgrading to go1.18 and then we decide.

@ribejara-te
Copy link
Contributor Author

What's the progress on that? Do we have an ETA on 1.18? @chenrui333

@skleiner-te
Copy link

@jamengual doesn't look like go1.18 is going to happen in the next week- could we go ahead with this as is?

@jamengual
Copy link
Contributor

jamengual commented Jul 12, 2022 via email

@jamengual jamengual added provider/github waiting-on-review Waiting for a review from a maintainer bug Something isn't working labels Jul 14, 2022
@ribejara-te
Copy link
Contributor Author

ribejara-te commented Jul 20, 2022

@jamengual can we proceed with the change?

@jamengual
Copy link
Contributor

@chenrui333 what do you think?

@ribejara-te
Copy link
Contributor Author

Hi, it's been 2 weeks since we agreed to waiting for 1 week before merging.

Can we please merge? Again, once we upgrade to go1.18 it'll be as easy as git revert 15ee16d7.

@shanesavoie
Copy link

Looking forward to getting this fixed. I have a repo with over 400 projects and enabling parallel would be great.

@jamengual
Copy link
Contributor

fair, I think once we are ready we will move it update it.

@jamengual jamengual merged commit e0e29bd into runatlantis:master Jul 27, 2022
@jamengual
Copy link
Contributor

jamengual commented Sep 8, 2022

@ribejara-te we updated to 1.19

@jamengual
Copy link
Contributor

do you think you can create a PR with git revert 15ee16d7 and give it a test?

@ribejara-te
Copy link
Contributor Author

Sure

@ribejara-te
Copy link
Contributor Author

You got it at #2521.

krrrr38 pushed a commit to krrrr38/atlantis that referenced this pull request Dec 16, 2022
…s#2348)

* fix: repository cloning race condition (runatlantis#2341)

* fix: switched from sync to golang.org/x/sync/semaphore

* fix: check value of sem.Acquire(...)
@nitrocode nitrocode added this to the v0.19.8 milestone Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working provider/github waiting-on-review Waiting for a review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parallel planning race condition under specific conditions
5 participants