Skip to content

Commit 995453a

Browse files
authored
Merge pull request #71 from gojek/fix/rr-issue
fix: round-robin issue when publishing
2 parents 3003195 + d70e1bb commit 995453a

6 files changed

Lines changed: 106 additions & 62 deletions

File tree

client.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,10 @@ var newClientFunc = defaultNewClientFunc()
2222
type Client struct {
2323
options *clientOptions
2424

25-
subscriptions map[string]*subscriptionMeta
26-
mqttClient mqtt.Client
27-
mqttClients map[string]*internalState
25+
subscriptions map[string]*subscriptionMeta
26+
mqttClient mqtt.Client
27+
mqttClients map[string]*internalState
28+
orderedClients []*internalState
2829

2930
publisher Publisher
3031
subscriber Subscriber
@@ -151,6 +152,7 @@ func (c *Client) stop() error {
151152

152153
c.mqttClient = nil
153154
c.mqttClients = nil
155+
c.orderedClients = nil
154156
}
155157

156158
return err

client_pool.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,18 @@ import (
99

1010
func (c *Client) initializeConnectionPool() {
1111
c.mqttClients = make(map[string]*internalState, c.options.poolSize)
12+
c.orderedClients = make([]*internalState, 0, c.options.poolSize)
1213

1314
for i := 0; i < c.options.poolSize; i++ {
1415
mqttOpts := toClientOptions(c, c.options, fmt.Sprintf("-%d", i))
1516
mqttClient := newClientFunc.Load().(func(*mqtt.ClientOptions) mqtt.Client)(mqttOpts)
1617

1718
poolID := fmt.Sprintf("%s-%d", c.options.clientID, i)
18-
c.mqttClients[poolID] = &internalState{
19+
ic := &internalState{
1920
client: mqttClient,
2021
subsCalled: make(generic.Set[string]),
2122
}
23+
c.mqttClients[poolID] = ic
24+
c.orderedClients = append(c.orderedClients, ic)
2225
}
2326
}

client_pool_test.go

Lines changed: 74 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -114,61 +114,90 @@ func TestPoolSize(t *testing.T) {
114114
}
115115

116116
func TestPoolPublish(t *testing.T) {
117-
poolSize := 3
118-
mockClients := make([]*mockMQTTClient, poolSize)
119-
mockTokens := make([]*mockToken, poolSize)
117+
tests := []struct {
118+
name string
119+
poolSize int
120+
publishCount int
121+
expectedCalls int
122+
}{
123+
{
124+
name: "Pool size 2 Publish 4",
125+
poolSize: 2,
126+
publishCount: 4,
127+
expectedCalls: 2,
128+
},
129+
{
130+
name: "Pool size 3 Publish 12",
131+
poolSize: 3,
132+
publishCount: 12,
133+
expectedCalls: 4,
134+
},
135+
{
136+
name: "Pool size 4 Publish 24",
137+
poolSize: 4,
138+
publishCount: 24,
139+
expectedCalls: 6,
140+
},
141+
}
120142

121-
for i := 0; i < poolSize; i++ {
122-
mockClients[i] = newMockMQTTClient(t)
123-
mockTokens[i] = newMockToken(t)
143+
for _, tt := range tests {
144+
t.Run(tt.name, func(t *testing.T) {
145+
mockTokens := make([]*mockToken, tt.poolSize)
146+
mockClients := make([]*mockMQTTClient, tt.poolSize)
124147

125-
mockTokens[i].On("Wait").Return(true)
126-
mockTokens[i].On("WaitTimeout", mock.Anything).Return(true)
127-
mockTokens[i].On("Error").Return(nil)
148+
for i := 0; i < tt.poolSize; i++ {
149+
mockClients[i] = newMockMQTTClient(t)
150+
mockTokens[i] = newMockToken(t)
128151

129-
mockClients[i].On("IsConnectionOpen").Return(true)
130-
mockClients[i].On("Connect").Return(mockTokens[i])
131-
mockClients[i].On("Disconnect", mock.Anything)
132-
mockClients[i].On("Publish", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(mockTokens[i])
133-
}
152+
mockTokens[i].On("Wait").Return(true)
153+
mockTokens[i].On("WaitTimeout", mock.Anything).Return(true)
154+
mockTokens[i].On("Error").Return(nil)
134155

135-
clientIndex := 0
136-
originalFunc := newClientFunc.Load()
137-
newClientFunc.Store(func(opts *mqtt.ClientOptions) mqtt.Client {
138-
client := mockClients[clientIndex%poolSize]
139-
clientIndex++
140-
return client
141-
})
142-
defer newClientFunc.Store(originalFunc)
156+
mockClients[i].On("IsConnectionOpen").Return(true)
157+
mockClients[i].On("Connect").Return(mockTokens[i])
158+
mockClients[i].On("Disconnect", mock.Anything)
159+
mockClients[i].On("Publish", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(mockTokens[i])
160+
}
143161

144-
client, err := NewClient(
145-
WithAddress("localhost", 1883),
146-
WithClientID("test"),
147-
WithPoolSize(poolSize),
148-
)
149-
assert.NoError(t, err)
150-
assert.NotNil(t, client)
162+
clientIndex := 0
163+
originalFunc := newClientFunc.Load()
164+
newClientFunc.Store(func(opts *mqtt.ClientOptions) mqtt.Client {
165+
client := mockClients[clientIndex%tt.poolSize]
166+
clientIndex++
167+
return client
168+
})
169+
defer newClientFunc.Store(originalFunc)
151170

152-
err = client.Start()
153-
assert.NoError(t, err)
171+
client, err := NewClient(
172+
WithAddress("localhost", 1883),
173+
WithClientID("test"),
174+
WithPoolSize(tt.poolSize),
175+
)
176+
assert.NoError(t, err)
177+
assert.NotNil(t, client)
154178

155-
for i := range poolSize {
156-
atomic.StoreInt64(&mockClients[i].publishCallCount, 0)
157-
}
179+
err = client.Start()
180+
assert.NoError(t, err)
158181

159-
expectedPublishes := 6
160-
for i := 0; i < expectedPublishes; i++ {
161-
err = client.Publish(context.Background(), "topic", []byte("test"))
162-
assert.NoError(t, err)
163-
}
182+
for i := range tt.poolSize {
183+
atomic.StoreInt64(&mockClients[i].publishCallCount, 0)
184+
}
164185

165-
totalPublishCount := int64(0)
166-
for i := range poolSize {
167-
totalPublishCount += atomic.LoadInt64(&mockClients[i].publishCallCount)
168-
}
186+
for i := 0; i < tt.publishCount; i++ {
187+
err = client.Publish(context.Background(), "topic", []byte("test"))
188+
assert.NoError(t, err)
189+
}
169190

170-
assert.Equal(t, int64(expectedPublishes), totalPublishCount, "Total publish count should match expected")
171-
assert.NoError(t, client.stop())
191+
totalPublishCount := int64(0)
192+
for i := range tt.poolSize {
193+
totalPublishCount += atomic.LoadInt64(&mockClients[i].publishCallCount)
194+
assert.Equal(t, int64(tt.expectedCalls), atomic.LoadInt64(&mockClients[i].publishCallCount), "Each client should have correct publish call count")
195+
}
196+
197+
assert.Equal(t, int64(tt.publishCount), totalPublishCount, "Total publish count should match expected")
198+
assert.NoError(t, client.stop())
199+
})
200+
}
172201
}
173202

174203
func TestPoolSubscribe(t *testing.T) {

client_resolver.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,12 +91,16 @@ func (c *Client) reloadClients(clients map[string]mqtt.Client) {
9191

9292
if len(clients) > 0 {
9393
ncs := make(map[string]*internalState, len(clients))
94+
orderedClients := make([]*internalState, 0, len(clients))
9495

9596
for k, cc := range clients {
96-
ncs[k] = &internalState{client: cc, subsCalled: generic.NewSet[string]()}
97+
ic := &internalState{client: cc, subsCalled: generic.NewSet[string]()}
98+
ncs[k] = ic
99+
orderedClients = append(orderedClients, ic)
97100
}
98101

99102
c.mqttClients = ncs
103+
c.orderedClients = orderedClients
100104
}
101105

102106
c.options.logger.Info(context.Background(), "reloading clients", map[string]any{

docs/docs/sdk/SDK.md

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ func WaitForConnection(c ConnectionInformer, waitFor time.Duration, tick time.Du
192192
WaitForConnection checks if the Client is connected, it calls ConnectionInformer.IsConnected after every tick and waitFor is the maximum duration it can block. Returns true only when ConnectionInformer.IsConnected returns true
193193

194194
<a name="Client"></a>
195-
## type [Client](https://github.com/gojek/courier-go/blob/main/client.go#L22-L45)
195+
## type [Client](https://github.com/gojek/courier-go/blob/main/client.go#L22-L46)
196196

197197
Client allows to communicate with an MQTT broker
198198

@@ -203,7 +203,7 @@ type Client struct {
203203
```
204204

205205
<a name="NewClient"></a>
206-
### func [NewClient](https://github.com/gojek/courier-go/blob/main/client.go#L50)
206+
### func [NewClient](https://github.com/gojek/courier-go/blob/main/client.go#L51)
207207

208208
```go
209209
func NewClient(opts ...ClientOption) (*Client, error)
@@ -262,7 +262,7 @@ c.Stop()
262262
</details>
263263

264264
<a name="Client.AckTimeout"></a>
265-
### func \(\*Client\) [AckTimeout](https://github.com/gojek/courier-go/blob/main/client.go#L407)
265+
### func \(\*Client\) [AckTimeout](https://github.com/gojek/courier-go/blob/main/client.go#L409)
266266

267267
```go
268268
func (c *Client) AckTimeout() time.Duration
@@ -271,7 +271,7 @@ func (c *Client) AckTimeout() time.Duration
271271
AckTimeout returns the ack timeout duration configured for the client
272272

273273
<a name="Client.ConnectTimeout"></a>
274-
### func \(\*Client\) [ConnectTimeout](https://github.com/gojek/courier-go/blob/main/client.go#L402)
274+
### func \(\*Client\) [ConnectTimeout](https://github.com/gojek/courier-go/blob/main/client.go#L404)
275275

276276
```go
277277
func (c *Client) ConnectTimeout() time.Duration
@@ -289,7 +289,7 @@ func (c *Client) InfoHandler() http.Handler
289289
InfoHandler returns a http.Handler that exposes the connected clients information
290290

291291
<a name="Client.IsConnected"></a>
292-
### func \(\*Client\) [IsConnected](https://github.com/gojek/courier-go/blob/main/client.go#L95)
292+
### func \(\*Client\) [IsConnected](https://github.com/gojek/courier-go/blob/main/client.go#L96)
293293

294294
```go
295295
func (c *Client) IsConnected() bool
@@ -298,7 +298,7 @@ func (c *Client) IsConnected() bool
298298
IsConnected checks whether the client is connected to the broker
299299

300300
<a name="Client.KeepAlive"></a>
301-
### func \(\*Client\) [KeepAlive](https://github.com/gojek/courier-go/blob/main/client.go#L392)
301+
### func \(\*Client\) [KeepAlive](https://github.com/gojek/courier-go/blob/main/client.go#L394)
302302

303303
```go
304304
func (c *Client) KeepAlive() time.Duration
@@ -307,7 +307,7 @@ func (c *Client) KeepAlive() time.Duration
307307
KeepAlive returns the keep alive duration configured for the client
308308

309309
<a name="Client.PoolSize"></a>
310-
### func \(\*Client\) [PoolSize](https://github.com/gojek/courier-go/blob/main/client.go#L412)
310+
### func \(\*Client\) [PoolSize](https://github.com/gojek/courier-go/blob/main/client.go#L414)
311311

312312
```go
313313
func (c *Client) PoolSize() int
@@ -325,7 +325,7 @@ func (c *Client) Publish(ctx context.Context, topic string, message interface{},
325325
Publish allows to publish messages to an MQTT broker
326326

327327
<a name="Client.Run"></a>
328-
### func \(\*Client\) [Run](https://github.com/gojek/courier-go/blob/main/client.go#L123)
328+
### func \(\*Client\) [Run](https://github.com/gojek/courier-go/blob/main/client.go#L124)
329329

330330
```go
331331
func (c *Client) Run(ctx context.Context) error
@@ -334,7 +334,7 @@ func (c *Client) Run(ctx context.Context) error
334334
Run will start running the Client. This makes Client compatible with github.com/gojekfarm/xrun package. https://pkg.go.dev/github.com/gojekfarm/xrun
335335

336336
<a name="Client.Start"></a>
337-
### func \(\*Client\) [Start](https://github.com/gojek/courier-go/blob/main/client.go#L108)
337+
### func \(\*Client\) [Start](https://github.com/gojek/courier-go/blob/main/client.go#L109)
338338

339339
```go
340340
func (c *Client) Start() error
@@ -343,7 +343,7 @@ func (c *Client) Start() error
343343
Start will attempt to connect to the broker.
344344

345345
<a name="Client.Stop"></a>
346-
### func \(\*Client\) [Stop](https://github.com/gojek/courier-go/blob/main/client.go#L119)
346+
### func \(\*Client\) [Stop](https://github.com/gojek/courier-go/blob/main/client.go#L120)
347347

348348
```go
349349
func (c *Client) Stop()
@@ -415,7 +415,7 @@ func (c *Client) UseUnsubscriberMiddleware(mwf ...UnsubscriberMiddlewareFunc)
415415
UseUnsubscriberMiddleware appends a UnsubscriberMiddlewareFunc to the chain. Middleware can be used to intercept or otherwise modify, process or skip subscriptions. They are executed in the order that they are applied to the Client.
416416

417417
<a name="Client.WriteTimeout"></a>
418-
### func \(\*Client\) [WriteTimeout](https://github.com/gojek/courier-go/blob/main/client.go#L397)
418+
### func \(\*Client\) [WriteTimeout](https://github.com/gojek/courier-go/blob/main/client.go#L399)
419419

420420
```go
421421
func (c *Client) WriteTimeout() time.Duration

exec.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,13 @@ func (c *Client) execute(f func(mqtt.Client) error, eo execOpt) error {
6464
}
6565

6666
func (c *Client) execMultiConn(f func(mqtt.Client) error, eo execOpt) error {
67-
ccs := xmap.Values(c.mqttClients)
67+
var ccs []*internalState
68+
69+
if eo == execOneRoundRobin {
70+
ccs = c.orderedClients
71+
} else {
72+
ccs = xmap.Values(c.mqttClients)
73+
}
6874

6975
switch eo := eo.(type) {
7076
case execOptConst:

0 commit comments

Comments
 (0)