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

test: Add mutex to opening PostgreSQL ports to prevent collision #389

Merged
merged 2 commits into from
Mar 1, 2022

Conversation

kylecarbs
Copy link
Member

Closes #388.

@kylecarbs kylecarbs self-assigned this Mar 1, 2022
@kylecarbs kylecarbs enabled auto-merge (squash) March 1, 2022 18:06
@codecov
Copy link

codecov bot commented Mar 1, 2022

Codecov Report

Merging #389 (d46072d) into main (6c2371e) will increase coverage by 0.20%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #389      +/-   ##
==========================================
+ Coverage   67.92%   68.13%   +0.20%     
==========================================
  Files         148      148              
  Lines        8297     8304       +7     
  Branches       72       72              
==========================================
+ Hits         5636     5658      +22     
+ Misses       2099     2088      -11     
+ Partials      562      558       -4     
Flag Coverage Δ
unittest-go-macos-latest 66.23% <ø> (+0.10%) ⬆️
unittest-go-ubuntu-latest 67.56% <60.00%> (+0.16%) ⬆️
unittest-js 66.10% <ø> (ø)
Impacted Files Coverage Δ
database/postgres/postgres.go 70.78% <60.00%> (+0.05%) ⬆️
peer/channel.go 81.87% <0.00%> (-0.59%) ⬇️
coderd/provisionerdaemons.go 58.36% <0.00%> (+1.06%) ⬆️
peerbroker/proxy.go 57.23% <0.00%> (+1.25%) ⬆️
peer/conn.go 78.77% <0.00%> (+1.53%) ⬆️
peerbroker/dial.go 80.95% <0.00%> (+4.76%) ⬆️

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 6c2371e...d46072d. Read the comment docs.

// Pick an explicit port on the host to connect to 5432.
// This is necessary so we can configure the port to only use ipv4.
port, err := getFreePort()
if err != nil {
openPortMutex.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Would it be safer to defer openPortMutex.Unlock() right after acquiring the lock? Just worried we may add a new if and forgot to unlock the mutex down the road.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had that initially but didn't want each execution to wait for the PostgreSQL instance to start up.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could separate out these functions though. I'm happy to do that if you feel it'd prevent a bug in the future!

@kylecarbs kylecarbs merged commit 7e72eb9 into main Mar 1, 2022
@kylecarbs kylecarbs deleted the dockerport branch March 1, 2022 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate test flake for allocating ports with PostgreSQL Docker
2 participants