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.

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

fix: custom fields cache concurrent writes #564

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: custom fields cache concurrent writes #564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions .changes/unreleased/Fixed-20250206-113217.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
kind: Fixed
body: Custom fields cache concurrent writes
time: 2025-02-06T11:32:17.345811+01:00
8 changes: 8 additions & 0 deletions commercetools/custom_fields.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@ import (
"fmt"
"strconv"
"strings"
"sync"
"time"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/labd/commercetools-go-sdk/platform"
)

var cacheTypes map[string]*platform.Type
var cacheTypesLock sync.Mutex

func CustomFieldSchema() *schema.Schema {
return &schema.Schema{
Expand Down Expand Up @@ -257,6 +259,7 @@ func getTypeResourceFromResourceData(ctx context.Context, client *platform.ByPro
// field. The type_id is cached to minimize API calls when multiple resource
// use the same type
func GetTypeResource(ctx context.Context, client *platform.ByProjectKeyRequestBuilder, typeId string) (*platform.Type, error) {
cacheTypesLock.Lock()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we rather lock the whole method at the start? So:

cacheTypesLock.Lock()
defer cacheTypesLock.Unlock()

Makes it a bit easier to read, and then we don't have multiple locks and unlocks in the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If CT is slow at doing lookups you block other calls to GetTypeResource, slowing down the provider.

Copy link
Member

Choose a reason for hiding this comment

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

What I might see happening here is that we get some calls in succession for the same type that has not yet been fetched, which would all see the cache slice be empty. They would then all unlock, and all call commercetools afterwards, replacing the entry at the type key in the map in succession. This would generally be what I would expect to happen during a terraform plan/apply, as that is when we fetch all the information. So I am unsure if the above would be a significant improvement over just locking it the first time.

Maybe a better alternative would be something like https://pkg.go.dev/sync#Map, which should take care of most of these issues

Having said that, your solution obviously does fix the bug :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sync.Map doesn't solve that issue on its own either. It only blocks on the read and write operations, not fetching.
Today, if two requests come in succession you get a panic, it's not blocking in the current form.

If you want other behaviour you would want to block on type id, and not globally either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could change the underlying implementation to use x/sync's singleflight.Group instead, or a similar package to do what you mention.

if cacheTypes == nil {
cacheTypes = make(map[string]*platform.Type)
}
Expand All @@ -266,9 +269,14 @@ func GetTypeResource(ctx context.Context, client *platform.ByProjectKeyRequestBu
}
return t, nil
}
cacheTypesLock.Unlock()

t, err := client.Types().WithId(typeId).Get().Execute(ctx)

cacheTypesLock.Lock()
cacheTypes[typeId] = t
cacheTypesLock.Unlock()

return t, err
}

Expand Down