feat: Implement connection manager #1021

sameh-farouk · 2024-11-17T22:13:30Z

Connection Pool Manager Implementation

Overview

This PR introduces a connection pool manager for TFChain client to improve connection handling, and reliability. It replaces the previous simple single connection management with a pool management implementation.

Key Features

Connection Pooling: Maintains a configurable pool of connections (min/max size)
Automatic Recovery: Handles failed connections with automatic replacement
Connection Reuse: Efficiently reuses healthy connections
Configurable Settings: Customizable pool size, idle timeout, and connection timeouts
Thread Safety: thread-safe implementation with lock handling
Resource Management: Proper cleanup of idle and unhealthy connections
Tests:Added comprehensive test suite including stress tests

Are these changes back-compatible with old code

The Manager interface has been updated with new methods. The changes were almost compatible with the old code by opting for having aliases for deprecated/renamed methods and setting defaults for the pool configuration. However:

It's important to call the new mgr.Close() method when done with the manager.
If the defaults are not working for you, you should add a custom configuration by constructing the manager using the new NewManagerWithConfig function.

Deprecated Methods

No immediate action is required atm but these methods may be removed in the future

mgr.Raw() is deprecated
mgr.Substrate() is depricated and replaced with mgr.GetConnection().
sub.Close() is deprecated and replaced with sub.Release().

Checklist:

My change requires a change to the documentation and I have updated it accordingly
I have added tests to cover my changes.

…for now

Eslam-Nawara · 2024-11-18T11:43:00Z

clients/tfchain-client-go/impl.go

+	urls := shuffle(m.unusedURLs())
+	urls = append(urls, m.urls...)


I don't get it here, isn't the urls returned from unusedURLs are already in m.urls? why not just use m.urls? and why shuffling them?

When creating new connections, I want to prioritize unused URLs but won’t restrict attempts to them if all unused URLs are down. also, unusedURLs() can return 0 URLs.
Shuffling was implemented to distribute loads. Without shuffling, when an RPC node goes down, all nodes would attempt to connect to the same URL, causing spikes and high load.

Eslam-Nawara · 2024-11-18T12:08:13Z

clients/tfchain-client-go/impl.go

+	if conn := m.getExistingConn(); conn != nil {
+		return conn, nil
+	}


we're trying to get an existing connection here and then retrying again in the backoff retry function, should we remove this and only use the one in the retry

rawdaGastan

didn't review tests
to be continued

rawdaGastan · 2024-11-18T09:13:59Z

clients/tfchain-client-go/impl.go

-	// be behind before we don't accept it. block time is 6 seconds, so
-	// right now we only allow 2 blocks delay
-	acceptableDelay = 2 * 6 * time.Second
+var (


why comments are removed

definitely restore the comments in the PR

I wanted to clean up my own comments but accidentally deleted old ones as well, thanks to dump code editor plugins :(

rawdaGastan · 2024-11-18T09:20:39Z

clients/tfchain-client-go/impl.go

+	return err == nil
+}
+
+func (pc *poolConn) close() {


don't we need to update lastUsed and isUse?

rawdaGastan · 2024-11-18T09:24:19Z

clients/tfchain-client-go/impl.go

+	// so instead we just check the health when we need to and do the maintanance in demand
+	// HealthCheckInterval time.Duration
+	// Timeout for creating new connections
+	ConnectionTimeout time.Duration


can't get it? why setting a timeout for creating connections?

Typically, this is the behavior you want.
Mainly to avoid waiting indefinitely on a stuck connection instead of attempting to reconnect.
Also, this parameter is not used at all and should be cleaned up.
The reason I removed the timeout logic (not completely as you noticed) is that I found it already implemented in the inner substrate client out of the box, so it wasn't needed in our code.

rawdaGastan · 2024-11-18T09:24:58Z

clients/tfchain-client-go/impl.go

+	// Minimum number of connections to maintain
+	MinPoolSize int
+	// Maximum time a connection can be idle before being closed (if the pool has more than MinPoolSize)
+	MaxIdleTime time.Duration


what happens if I reached it?

@rawdaGastan the connection should get closed as far as i understand

rawdaGastan · 2024-11-18T09:26:09Z

clients/tfchain-client-go/impl.go

+
+type manager struct {
+	urls      []string
+	pool      []*poolConn


rawdaGastan · 2024-11-18T12:04:44Z

clients/tfchain-client-go/impl.go

+	t, err := getTime(cl, meta)
+	if err != nil || time.Since(t) > AcceptableDelay {
+		cl.Client.Close()
+		return nil, nil, fmt.Errorf("node health check failed")


why not embedding the error?

rawdaGastan · 2024-11-18T12:06:41Z

clients/tfchain-client-go/impl.go

+	if s.conn != nil && s.conn.isHealthy() {
+		conn := s.conn.conn
+		meta := s.conn.meta
+		s.conn.lastUsed.Store(time.Now().Unix())


should we update inUse field here?

rawdaGastan · 2024-11-18T12:14:29Z

clients/tfchain-client-go/impl.go

+		Msg("checking for healthy connections")
+
+	// Try getting existing connection first
+	if conn := m.getExistingConn(); conn != nil {


why trying to get connection before using backoff.Retry?

rawdaGastan · 2024-11-18T12:23:01Z

clients/tfchain-client-go/impl.go

+	return unused
+}
+
+func (m *manager) aquiredConnCount() int {


I think it ia about inUse count not all connections>

rawdaGastan · 2024-11-18T12:24:32Z

clients/tfchain-client-go/impl.go

+	log.Debug().Msg("ensuring minimum connections in the pool")
+	inUseCount := m.aquiredConnCount()
+	urls := shuffle(m.unusedURLs())
+	urls = append(urls, m.urls...)


why appending shuffled unused and all manager urls?

see my comment here

AbdelrahmanElawady · 2024-11-18T13:08:09Z

clients/tfchain-client-go/impl.go

+}
+
+func (m *manager) getHealthyConn() (*poolConn, error) {
+	log.Debug().Int("pool_size", len(m.pool)).Int("aquired_count", m.aquiredConnCount()).


btw m.pool is accessed here without any locking which leads to data race. Try running the tests with -race flag

also typo aquired should be acquired

if it does an io, it should have a context.

Eslam-Nawara · 2024-11-18T13:12:08Z

clients/tfchain-client-go/impl_test.go

+	mgrImpl.mu.RLock()
+	defer mgrImpl.mu.RUnlock()
+
+	assert.LessOrEqual(t, len(mgrImpl.pool), mgrImpl.config.MinPoolSize)


shouldn't len(mgrImpl.pool) be greater or equal the the MinPoolSize?

xmonader · 2024-11-18T13:48:16Z

clients/tfchain-client-go/impl.go

-	// right now we only allow 2 blocks delay
-	acceptableDelay = 2 * 6 * time.Second
+var (
+	ErrInvalidVersion         = fmt.Errorf("invalid version")


For static errors use only errors.New, fmt.Errorf communicates the intention of either wrapping or formatting

xmonader · 2024-11-18T13:48:52Z

clients/tfchain-client-go/impl.go

-	//ErrNotFound is returned if an object is not found
-	ErrNotFound = fmt.Errorf("object not found")
+const (
+	AcceptableDelay = 2 * 6 * time.Second


WHy exported?

and restore the comment please

Indeed shouldn't be exported since it's an internal implementation

xmonader · 2024-11-18T13:50:04Z

clients/tfchain-client-go/impl.go

@@ -37,145 +36,437 @@ type Versioned struct {
 type Conn = *gsrpc.SubstrateAPI


This alias is misleading.

What are your suggestions here?

xmonader · 2024-11-18T13:50:10Z

clients/tfchain-client-go/impl.go

@@ -37,145 +36,437 @@ type Versioned struct {
 type Conn = *gsrpc.SubstrateAPI
 type Meta = *types.Metadata


xmonader · 2024-11-18T14:55:02Z

clients/tfchain-client-go/impl.go

+			conn:     conn,
+			meta:     meta,
+			url:      url,
+			lastUsed: atomic.Int64{},


Initialized to what? use .Store with unix timestamp

xmonader · 2024-11-18T14:55:10Z

clients/tfchain-client-go/impl.go

+			meta:     meta,
+			url:      url,
+			lastUsed: atomic.Int64{},
+			inUse:    atomic.Bool{},


Use .Store(false)

xmonader · 2024-11-18T14:59:38Z

clients/tfchain-client-go/impl.go

-		url[i], url[j] = url[j], url[i]
-	})
+	// Validate and adjust configuration
+	if config.MaxPoolSize < 1 {


these validations should fail and never swap, override values provided by the user, we need to tell them they're wrong instead

xmonader · 2024-11-18T15:03:08Z

clients/tfchain-client-go/impl.go

+		ctx:       ctx,
+		cancel:    cancel,
+		config:    config,
+		checkChan: make(chan struct{}, 1),


Why buffered?

The buffer size of 1 is sufficient because we only need to know if a check is pending or not.
If a check is already pending, other goroutines are not blocked (Non-blocking sends).
Also, Multiple check requests are coalesced, collapsing multiple requests into a single check operation (reduces contention).

select { case m.checkChan <- struct{}{}: // Non-blocking send log.Debug().Msg("triggered connection check") default: // If channel is full, skip log.Debug().Msg("connection check already pending") }

Note that initially, I have the health check execute on demand—immediately when a connection fails—and periodically, however, I later reconsidered the impact of the side effects on the load and the actual benefits.

xmonader · 2024-11-18T16:48:40Z

clients/tfchain-client-go/impl.go

-	if len(url) == 0 {
-		panic("at least one url is required")
+func NewManagerWithConfig(config ManagerConfig, urls ...string) Manager {
+	if len(urls) == 0 {


never panic on input validation!

xmonader · 2024-11-18T16:50:41Z

clients/tfchain-client-go/impl.go

+	m := &manager{
+		urls:      shuffle(urls),
+		pool:      make([]*poolConn, 0, config.MaxPoolSize),
+		ctx:       ctx,


Redflag to store ctx on a struct.

xmonader · 2024-11-18T16:51:05Z

clients/tfchain-client-go/impl.go

+	urls      []string
+	pool      []*poolConn
+	mu        sync.RWMutex
+	ctx       context.Context


I doubt we need that, it may even introduce more problems than it fixes

xmonader · 2024-11-18T16:51:54Z

clients/tfchain-client-go/impl.go

-// the connection must be closed after you are done using it
-func (p *mgrImpl) Substrate() (*Substrate, error) {
-	cl, meta, err := p.Raw()
+func (m *manager) GetConnection(ctx context.Context) (*Substrate, error) {


Where is that ctx used?

xmonader · 2024-11-18T17:02:48Z

clients/tfchain-client-go/impl.go

+		poolSize := len(m.pool)
+
+		if poolSize < m.config.MinPoolSize || (poolSize < m.config.MaxPoolSize && poolSize == inUseCount) {
+			if conn, err := m.createConnection(url); err == nil {


and in case of error?

xmonader · 2024-11-18T17:03:37Z

clients/tfchain-client-go/impl.go

+func (s *Substrate) Close() {
+	s.Release()
+}
+
 func getTime(cl Conn, meta Meta) (t time.Time, err error) {


Should have context I believe.

xmonader · 2024-11-18T17:06:18Z

clients/tfchain-client-go/impl.go

+	if s.conn != nil {
+		s.conn.inUse.Store(false)
+	}
+	s.mu.Unlock()


there's unlock case in 434, and always unlock on 440?

xmonader · 2024-11-18T17:09:04Z

clients/tfchain-client-go/impl.go

-	meta Meta
+func (m *manager) Close() error {
+	m.cancel()
+	m.wg.Wait()


This assumes we only created it with NewManagerWithConfig and may block forever.

xmonader · 2024-11-18T17:13:00Z

clients/tfchain-client-go/impl.go

+	return conn, err
+}
+
+func (m *manager) healthChecker() {


for { m.mu.Lock() for len(m.pool) == 0 { // Wait until there is work to do or the context is canceled m.cond.Wait() } m.mu.Unlock() // Perform health checks select { case <-m.ctx.Done(): return default: m.checkConnections() } }

xmonader · 2024-11-18T17:13:23Z

clients/tfchain-client-go/impl.go

+	cancel    context.CancelFunc
+	wg        sync.WaitGroup
+	config    ManagerConfig
+	checkChan chan struct{}


replace with a condition variable instead

xmonader · 2024-11-18T17:21:49Z

clients/tfchain-client-go/impl.go

+}
+
+// Default configuration
+var DefaultConfig = ManagerConfig{


While it's common I'd prefer to have SubstrateManagerDefaultConfig as a function instead

xmonader · 2024-11-18T17:27:31Z

clients/tfchain-client-go/impl.go

+	conn     Conn
+	meta     Meta
+	url      string
+	lastUsed atomic.Int64 // Unix timestamp


I don't believe the poolConn is used anywhere without the mutex, so probably better to get rid of all of the atomics.

iwanbk · 2024-11-19T03:54:25Z

There are few things that i noticed here:

why we need to shuffle? Because it also considering the url used by the pool element. Which means it eventually become round robin.
looks like initializePool is not needed. We can initialize checkChan without buffer, then we can remove the initializePool
Do we really need to have ready-to-use conn? That we need to have MinPoolSize conn. It has some costs:

code complexity
unnecessary connection creation -> unnecessary CPU usage on both server & client

Do we really need to have this pool in the first place? If we use http, i've made some check:

It has concurrent request test, which indicate that a single client can be used concurrently
https://github.com/centrifuge/go-substrate-rpc-client/blob/master/gethrpc/client_test.go#L394-L407
use single Go http.Client, which already has connection pool inside

https://github.com/centrifuge/go-substrate-rpc-client/blob/e3b938563803c6a71043c4e7ba2e0d27c400f514/gethrpc/http.go#L124-L126

I did a quick read on the websocket, but not sure about it.

If we use http, then i think we can simply use one substrate client for each URL

Please double check the substrate client internal

iwanbk · 2024-11-19T13:07:11Z

OK, found out that we use websocket.

I did a quick read on the websocket, but not sure about it.

We indeed need to have a pool if we use websocket, because it doesn't have the internal pool like the http one.

iwanbk · 2024-11-19T13:20:24Z

but the docs says the contrary: it is websocket which can do concurrent request

https://docs.substrate.io/build/application-development/#connecting-to-a-node

Most applications use a WebSocket port because a single connection can be used for multiple messages to and from a node. With an HTTP connection, applications can only send and receive responses one message at a time

sameh-farouk added 13 commits November 17, 2024 23:43

feat: Implement connection manager

bac9f13

fix go linter

bb36e80

Adjust tests to use one url to work with substarte integration tests …

19211f4

…for now

Respect CI env like in other tests

2ffd833

set MaxElapsedTime for the backoff

a851f2a

update utils_test.go

e4cc602

update go veriosn in lint workflow

91f8e87

update go veriosn and golangci-lint-action in lint workflow

db1111b

update staticcheck-action in go lint workflow

f949e1f

remove obsolete client workflows

b1aea28

Update go version in build_test docker image

a812cd8

bump build_test image version to 5

3b6ec40

use atomic.Bool for closed bool

2cfa156

sameh-farouk marked this pull request as ready for review November 18, 2024 03:24

sameh-farouk requested review from renauter and LeeSmet as code owners November 18, 2024 03:24

xmonader requested a review from iwanbk November 18, 2024 11:07

Eslam-Nawara reviewed Nov 18, 2024

View reviewed changes

rawdaGastan requested changes Nov 18, 2024

View reviewed changes

AbdelrahmanElawady reviewed Nov 18, 2024

View reviewed changes

Eslam-Nawara reviewed Nov 18, 2024

View reviewed changes

xmonader reviewed Nov 18, 2024

View reviewed changes

		urls := shuffle(m.unusedURLs())
		urls = append(urls, m.urls...)

		@@ -37,145 +36,437 @@ type Versioned struct {
		type Conn = *gsrpc.SubstrateAPI

		@@ -37,145 +36,437 @@ type Versioned struct {
		type Conn = *gsrpc.SubstrateAPI
		type Meta = *types.Metadata

feat: Implement connection manager #1021

Are you sure you want to change the base?

feat: Implement connection manager #1021

Conversation

sameh-farouk commented Nov 17, 2024 • edited Loading

Connection Pool Manager Implementation

Overview

Key Features

Are these changes back-compatible with old code

Deprecated Methods

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rawdaGastan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmonader Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmonader Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

xmonader Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwanbk commented Nov 19, 2024

iwanbk commented Nov 19, 2024

iwanbk commented Nov 19, 2024

sameh-farouk commented Nov 17, 2024 •

edited

Loading

xmonader Nov 18, 2024 •

edited

Loading

xmonader Nov 18, 2024 •

edited

Loading

xmonader Nov 18, 2024 •

edited

Loading