Fix memory leak by reusing Odoo client (#209)

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()`.

Author: bastjan

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)

apiserver/billing/odoostorage/odoo/odoo16/odoo16_test.go

@@ -6,14 +6,14 @@ import (
 	"testing"
 	"time"
 
+	odooclient "github.com/appuio/go-odoo"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/mock/gomock"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	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)
+}