From 911b3bb720a2da42b39df9a6f31d134001fd3244 Mon Sep 17 00:00:00 2001 From: Sebastian Widmer Date: Thu, 4 Jul 2024 16:47:45 +0200 Subject: [PATCH] Fix memory leak by reusing Odoo client The Odoo client uses http.Client under the hood which should be reused since it keeps connections open. The initialization of the Odoo client is not thread-safe so I added a wrapper that fully initializes the client before any parallelized call. See comments on `FullInitialization()`. --- .../billing/odoostorage/odoo/odoo16/odoo16.go | 79 +++++++++++++++++-- .../odoostorage/odoo/odoo16/odoo16_test.go | 42 +++++++++- .../odoo/odoo16/odoo16mock/odoo16.go | 14 ++++ 3 files changed, 128 insertions(+), 7 deletions(-) diff --git a/apiserver/billing/odoostorage/odoo/odoo16/odoo16.go b/apiserver/billing/odoostorage/odoo/odoo16/odoo16.go index b0ec391e..7feeef70 100644 --- a/apiserver/billing/odoostorage/odoo/odoo16/odoo16.go +++ b/apiserver/billing/odoostorage/odoo/odoo16/odoo16.go @@ -7,6 +7,7 @@ import ( "fmt" "strconv" "strings" + "sync" "time" "github.com/google/uuid" @@ -68,20 +69,82 @@ type Config struct { var _ odoo.OdooStorage = &Odoo16Storage{} +// NewOdoo16Storage returns a new storage provider for BillingEntities +// The storage provider uses Odoo 16 as the backend. func NewOdoo16Storage(credentials OdooCredentials, conf Config) *Odoo16Storage { return &Odoo16Storage{ config: conf, - sessionCreator: func(ctx context.Context) (Odoo16Client, error) { - return odooclient.NewClient(&credentials) - }, + sessionCreator: CachingClientCreator(func(ctx context.Context) (Odoo16Client, error) { + c, err := odooclient.NewClient(&credentials) + return &OdooClientWithFullInitialization{c}, err + }), } } func NewFailedRecordScrubber(credentials OdooCredentials) *FailedRecordScrubber { return &FailedRecordScrubber{ - sessionCreator: func(ctx context.Context) (Odoo16Client, error) { - return odooclient.NewClient(&credentials) - }, + sessionCreator: CachingClientCreator(func(ctx context.Context) (Odoo16Client, error) { + c, err := odooclient.NewClient(&credentials) + return &OdooClientWithFullInitialization{c}, err + }), + } +} + +// OdooClientWithFullInitialization is a wrapper around the Odoo client that provides a FullInitialization method. +type OdooClientWithFullInitialization struct { + *odooclient.Client +} + +// FullInitialization is a workaround for the odooclient initialization which is not thread-safe. +// This function performs a full initialization of the client by calling execute_kw which initializes the "object" client. +// https://github.com/appuio/go-odoo/blob/a2a337fdf12becaeaa920ee8093402d6d01144f3/odoo.go#L337 +// After this call the client should be thread-safe and ready to use. +// The "common" client is already initialized in the NewClient function through the call of "authenticate". +// https://github.com/appuio/go-odoo/blob/a2a337fdf12becaeaa920ee8093402d6d01144f3/odoo.go#L53 +// https://github.com/appuio/go-odoo/blob/a2a337fdf12becaeaa920ee8093402d6d01144f3/odoo.go#L346 +func (c *OdooClientWithFullInitialization) FullInitialization() error { + _, err := c.Client.ExecuteKw("search_count", odooclient.ResPartnerModel, []any{*odooclient.NewCriteria()}, nil) + return err +} + +// CachingClientCreator accepts and returns a function that creates a new Odoo16Client instance. +// The function caches the client instance returned from the upstream client creation function and returns it on subsequent calls. +// If an error occurs during the creation of the client, the error is returned and the client creation is retried on the next call. +// No client is cached if an error occurs during the creation. +// +// This function is useful to avoid creating a new client instance for every request. +// Odoo clients track connections and session state internally, so reusing a client instance is beneficial. +// The upstream Odoo client is not thread-safe until a full initialization is performed. +// See the FullInitialization method on OdooClientWithFullInitialization for a workaround. +func CachingClientCreator(sc func(context.Context) (Odoo16Client, error)) func(context.Context) (Odoo16Client, error) { + clientMux := sync.RWMutex{} + var client Odoo16Client + + return func(ctx context.Context) (Odoo16Client, error) { + clientMux.RLock() + if client != nil { + clientMux.RUnlock() + return client, nil + } + clientMux.RUnlock() + + clientMux.Lock() + defer clientMux.Unlock() + + // recheck if client was created between RUnlock and Lock + if client != nil { + return client, nil + } + + c, err := sc(ctx) + if err != nil { + return nil, err + } + if err := c.FullInitialization(); err != nil { + return nil, fmt.Errorf("error during full initialization: %w", err) + } + client = c + return client, nil } } @@ -97,6 +160,10 @@ type FailedRecordScrubber struct { //go:generate go run go.uber.org/mock/mockgen -destination=./odoo16mock/$GOFILE -package odoo16mock . Odoo16Client type Odoo16Client interface { + // FullInitialization performs a full initialization of the client. + // After this call the client must be thread-safe and ready to use. + FullInitialization() error + Update(string, []int64, interface{}) error FindResPartners(*odooclient.Criteria, *odooclient.Options) (*odooclient.ResPartners, error) CreateResPartner(*odooclient.ResPartner) (int64, error) diff --git a/apiserver/billing/odoostorage/odoo/odoo16/odoo16_test.go b/apiserver/billing/odoostorage/odoo/odoo16/odoo16_test.go index f3a3d093..b6dfab69 100644 --- a/apiserver/billing/odoostorage/odoo/odoo16/odoo16_test.go +++ b/apiserver/billing/odoostorage/odoo/odoo16/odoo16_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + odooclient "github.com/appuio/go-odoo" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" @@ -13,7 +14,6 @@ import ( billingv1 "github.com/appuio/control-api/apis/billing/v1" "github.com/appuio/control-api/apiserver/billing/odoostorage/odoo/odoo16/odoo16mock" - odooclient "github.com/appuio/go-odoo" ) func TestGet(t *testing.T) { @@ -398,3 +398,43 @@ func TestCleanup(t *testing.T) { require.NoError(t, err) } + +func Test_CachingClientCreator(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + calls := 0 + shouldFail := true + + subject := CachingClientCreator(func(ctx context.Context) (Odoo16Client, error) { + calls++ + if shouldFail { + return nil, errors.New("failed to create client") + } + mock := odoo16mock.NewMockOdoo16Client(ctrl) + mock.EXPECT().FullInitialization().Times(1).Return(nil) + return mock, nil + }) + + // Failing call should return an error + _, err := subject(context.Background()) + require.Error(t, err) + assert.Equal(t, 1, calls) + _, err = subject(context.Background()) + require.Error(t, err) + assert.Equal(t, 2, calls) + + shouldFail = false + + // First successful call should create a new client + client, err := subject(context.Background()) + require.NoError(t, err) + assert.Equal(t, 3, calls) + prevClient := client + + // Second successful call should return the same client + client, err = subject(context.Background()) + require.NoError(t, err) + assert.Equal(t, prevClient, client) + assert.Equal(t, 3, calls) +} diff --git a/apiserver/billing/odoostorage/odoo/odoo16/odoo16mock/odoo16.go b/apiserver/billing/odoostorage/odoo/odoo16/odoo16mock/odoo16.go index 1b52fe22..00ad40d4 100644 --- a/apiserver/billing/odoostorage/odoo/odoo16/odoo16mock/odoo16.go +++ b/apiserver/billing/odoostorage/odoo/odoo16/odoo16mock/odoo16.go @@ -82,6 +82,20 @@ func (mr *MockOdoo16ClientMockRecorder) FindResPartners(arg0, arg1 any) *gomock. return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FindResPartners", reflect.TypeOf((*MockOdoo16Client)(nil).FindResPartners), arg0, arg1) } +// FullInitialization mocks base method. +func (m *MockOdoo16Client) FullInitialization() error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "FullInitialization") + ret0, _ := ret[0].(error) + return ret0 +} + +// FullInitialization indicates an expected call of FullInitialization. +func (mr *MockOdoo16ClientMockRecorder) FullInitialization() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FullInitialization", reflect.TypeOf((*MockOdoo16Client)(nil).FullInitialization)) +} + // Update mocks base method. func (m *MockOdoo16Client) Update(arg0 string, arg1 []int64, arg2 any) error { m.ctrl.T.Helper()