From fa3a3dbd82b02a3d8509aa809f09168df7f23851 Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Fri, 17 Jun 2022 11:34:46 -0700 Subject: [PATCH 1/4] Lock the fake database during transactions Signed-off-by: Spike Curtis --- coderd/database/databasefake/databasefake.go | 71 +++++++++++++------- 1 file changed, 47 insertions(+), 24 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 93adedf9e87b3..21003ce56d0de 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -18,33 +18,54 @@ import ( // New returns an in-memory fake of the database. func New() database.Store { return &fakeQuerier{ - apiKeys: make([]database.APIKey, 0), - organizationMembers: make([]database.OrganizationMember, 0), - organizations: make([]database.Organization, 0), - users: make([]database.User, 0), - - auditLogs: make([]database.AuditLog, 0), - files: make([]database.File, 0), - gitSSHKey: make([]database.GitSSHKey, 0), - parameterSchemas: make([]database.ParameterSchema, 0), - parameterValues: make([]database.ParameterValue, 0), - provisionerDaemons: make([]database.ProvisionerDaemon, 0), - provisionerJobAgents: make([]database.WorkspaceAgent, 0), - provisionerJobLogs: make([]database.ProvisionerJobLog, 0), - provisionerJobResources: make([]database.WorkspaceResource, 0), - provisionerJobs: make([]database.ProvisionerJob, 0), - templateVersions: make([]database.TemplateVersion, 0), - templates: make([]database.Template, 0), - workspaceBuilds: make([]database.WorkspaceBuild, 0), - workspaceApps: make([]database.WorkspaceApp, 0), - workspaces: make([]database.Workspace, 0), - } -} + mutex: &sync.RWMutex{}, + data: &data{ + apiKeys: make([]database.APIKey, 0), + organizationMembers: make([]database.OrganizationMember, 0), + organizations: make([]database.Organization, 0), + users: make([]database.User, 0), + + auditLogs: make([]database.AuditLog, 0), + files: make([]database.File, 0), + gitSSHKey: make([]database.GitSSHKey, 0), + parameterSchemas: make([]database.ParameterSchema, 0), + parameterValues: make([]database.ParameterValue, 0), + provisionerDaemons: make([]database.ProvisionerDaemon, 0), + provisionerJobAgents: make([]database.WorkspaceAgent, 0), + provisionerJobLogs: make([]database.ProvisionerJobLog, 0), + provisionerJobResources: make([]database.WorkspaceResource, 0), + provisionerJobs: make([]database.ProvisionerJob, 0), + templateVersions: make([]database.TemplateVersion, 0), + templates: make([]database.Template, 0), + workspaceBuilds: make([]database.WorkspaceBuild, 0), + workspaceApps: make([]database.WorkspaceApp, 0), + workspaces: make([]database.Workspace, 0), + }, + } +} + +type rwMutex interface { + Lock() + RLock() + Unlock() + RUnlock() +} + +// inTxMutex is a no op, since inside a transaction we are already locked. +type inTxMutex struct{} + +func (_ *inTxMutex) Lock() {} +func (_ *inTxMutex) RLock() {} +func (_ *inTxMutex) Unlock() {} +func (_ *inTxMutex) RUnlock() {} // fakeQuerier replicates database functionality to enable quick testing. type fakeQuerier struct { - mutex sync.RWMutex + mutex rwMutex + *data +} +type data struct { // Legacy tables apiKeys []database.APIKey organizations []database.Organization @@ -73,7 +94,9 @@ type fakeQuerier struct { // InTx doesn't rollback data properly for in-memory yet. func (q *fakeQuerier) InTx(fn func(database.Store) error) error { - return fn(q) + q.mutex.Lock() + defer q.mutex.Unlock() + return fn(&fakeQuerier{mutex: &inTxMutex{}, data: q.data}) } func (q *fakeQuerier) AcquireProvisionerJob(_ context.Context, arg database.AcquireProvisionerJobParams) (database.ProvisionerJob, error) { From 945eed20c57fdaecae59099f6d32e1b06f2671ab Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Fri, 17 Jun 2022 12:00:22 -0700 Subject: [PATCH 2/4] Add ut for fake database transactions Signed-off-by: Spike Curtis --- .../databasefake/databasefake_test.go | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/coderd/database/databasefake/databasefake_test.go b/coderd/database/databasefake/databasefake_test.go index 6a3416b59c7ac..21c88c1f0261d 100644 --- a/coderd/database/databasefake/databasefake_test.go +++ b/coderd/database/databasefake/databasefake_test.go @@ -1,15 +1,67 @@ package databasefake_test import ( + "context" + "database/sql" "fmt" "reflect" "testing" + "time" + + "github.com/stretchr/testify/assert" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/database/databasefake" ) +// test that transactions don't deadlock, and that we don't see intermediate state. +func TestInTx(t *testing.T) { + t.Parallel() + + uut := databasefake.New() + + inTx := make(chan any) + queriesDone := make(chan any) + queriesStarted := make(chan any) + go func() { + err := uut.InTx(func(tx database.Store) error { + close(inTx) + _, err := tx.InsertOrganization(context.Background(), database.InsertOrganizationParams{ + Name: "1", + }) + assert.NoError(t, err) + <-queriesStarted + time.Sleep(5 * time.Millisecond) + _, err = tx.InsertOrganization(context.Background(), database.InsertOrganizationParams{ + Name: "2", + }) + assert.NoError(t, err) + return nil + }) + assert.NoError(t, err) + }() + var nums []int + go func() { + <-inTx + for i := 0; i < 20; i++ { + orgs, err := uut.GetOrganizations(context.Background()) + if err != nil { + assert.ErrorIs(t, err, sql.ErrNoRows) + } + nums = append(nums, len(orgs)) + time.Sleep(time.Millisecond) + } + close(queriesDone) + }() + close(queriesStarted) + <-queriesDone + // ensure we never saw 1 org, only 0 or 2. + for i := 0; i < 20; i++ { + assert.NotEqual(t, 1, nums[i]) + } +} + // TestExactMethods will ensure the fake database does not hold onto excessive // functions. The fake database is a manual implementation, so it is possible // we forget to delete functions that we remove. This unit test just ensures From 6299d0c2004dc8019283c5ba1dfd494ff65a93ef Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Fri, 17 Jun 2022 13:00:37 -0700 Subject: [PATCH 3/4] fix lint Signed-off-by: Spike Curtis --- coderd/database/databasefake/databasefake.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 21003ce56d0de..bb9f65df3cd9c 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -54,10 +54,10 @@ type rwMutex interface { // inTxMutex is a no op, since inside a transaction we are already locked. type inTxMutex struct{} -func (_ *inTxMutex) Lock() {} -func (_ *inTxMutex) RLock() {} -func (_ *inTxMutex) Unlock() {} -func (_ *inTxMutex) RUnlock() {} +func (inTxMutex) Lock() {} +func (inTxMutex) RLock() {} +func (inTxMutex) Unlock() {} +func (inTxMutex) RUnlock() {} // fakeQuerier replicates database functionality to enable quick testing. type fakeQuerier struct { @@ -96,7 +96,7 @@ type data struct { func (q *fakeQuerier) InTx(fn func(database.Store) error) error { q.mutex.Lock() defer q.mutex.Unlock() - return fn(&fakeQuerier{mutex: &inTxMutex{}, data: q.data}) + return fn(&fakeQuerier{mutex: inTxMutex{}, data: q.data}) } func (q *fakeQuerier) AcquireProvisionerJob(_ context.Context, arg database.AcquireProvisionerJobParams) (database.ProvisionerJob, error) { From 7da91abcd3786de67a31b756307c8763822e78b9 Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Fri, 17 Jun 2022 13:02:13 -0700 Subject: [PATCH 4/4] Fix lint macOS Signed-off-by: Spike Curtis --- agent/reaper/reaper_stub.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/reaper/reaper_stub.go b/agent/reaper/reaper_stub.go index 9021165c975d8..b7516b76f59b2 100644 --- a/agent/reaper/reaper_stub.go +++ b/agent/reaper/reaper_stub.go @@ -14,6 +14,6 @@ func IsInitProcess() bool { return false } -func ForkReap(pids reap.PidCh) error { +func ForkReap(_ reap.PidCh) error { return nil }