Skip to content

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

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

Merged
merged 2 commits into from
Mar 1, 2022
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d9f4a57
test: Add mutex to opening PostgreSQL ports to prevent collision
kylecarbs Mar 1, 2022
d46072d
Merge branch 'main' into dockerport
kylecarbs Mar 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions database/postgres/postgres.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,18 @@ import (
"io/ioutil"
"net"
"os"
"sync"
"time"

"github.com/ory/dockertest/v3"
"github.com/ory/dockertest/v3/docker"
"golang.org/x/xerrors"
)

// Required to prevent port collision during container creation.
// Super unlikely, but it happened. See: https://github.com/coder/coder/runs/5375197003
var openPortMutex sync.Mutex

// Open creates a new PostgreSQL server using a Docker container.
func Open() (string, func(), error) {
pool, err := dockertest.NewPool("")
Expand All @@ -23,10 +28,12 @@ func Open() (string, func(), error) {
if err != nil {
return "", nil, xerrors.Errorf("create tempdir: %w", err)
}
openPortMutex.Lock()
// 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!

return "", nil, xerrors.Errorf("Unable to get free port: %w", err)
}

Expand Down Expand Up @@ -64,8 +71,11 @@ func Open() (string, func(), error) {
config.RestartPolicy = docker.RestartPolicy{Name: "no"}
})
if err != nil {
openPortMutex.Unlock()
return "", nil, xerrors.Errorf("could not start resource: %w", err)
}
openPortMutex.Unlock()

hostAndPort := resource.GetHostPort("5432/tcp")
dbURL := fmt.Sprintf("postgres://postgres:postgres@%s/postgres?sslmode=disable", hostAndPort)

Expand Down