support multiple relays in rmb client #947
Conversation
rawdaGastan
requested review from
muhamadazmy,
mariobassem,
ashraffouda,
Omarabdul3ziz,
AbdelrahmanElawady,
xmonader,
sameh-farouk and
Eslam-Nawara
as code owners
…-sdk-go into development_multi_relays_support
…ter serve error before getting values from ctx (avoid panics)
rawdaGastan
force-pushed
the
development_multi_relays_support
branch
from
00bcbac to
2c2075d
Compare
rawdaGastan
force-pushed
the
development_multi_relays_support
branch
from
1c7569b to
3b0f53b
Compare
| "test": "wss://relay.test.grid.tf", | ||
| "qa": "wss://relay.qa.grid.tf", | ||
| "main": "wss://relay.grid.tf", | ||
| RelayURLS = map[string][]string{ |
There was a problem hiding this comment.
Use the same ones defined here
https://github.com/threefoldtech/zos/blob/main/pkg/environment/environment.go#L103
rmb-sdk-go/peer/connection.go
Outdated
| @@ -73,7 +81,28 @@ func (c *InnerConnection) reader(ctx context.Context, cancel context.CancelFunc, | |||
| } | |||
| } | |||
|
|
|||
| func (c *InnerConnection) loop(ctx context.Context, con *websocket.Conn, output, input chan []byte) error { | |||
| func (c *InnerConnection) send(ctx context.Context, data []byte) error { | |||
| if c.busy { | |||
There was a problem hiding this comment.
I suggest u use an atomic bool like here https://pkg.go.dev/sync/atomic#Bool so we don't run into race conditions
rmb-sdk-go/peer/connection.go
Outdated
| defer func() { | ||
| c.busy = false | ||
| }() |
There was a problem hiding this comment.
You can't use a defer in a loop. this is wrong. Read about it
This is why u still had to set c.busy=false later in the code at line (152)
I suggest that u move this entire branch logic to a function so u can use the defer to release the busy flag.
There was a problem hiding this comment.
As i remember the busy flag was intended to make a quick return if the connection is not established so senders can already try another connection until this one is restored. It means it shouldn't be turned on/off inside the branch, but instead ONLY when that connection instance is reconnecting to relay.
rmb-sdk-go/peer/peer.go
Outdated
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| var errs error | ||
| for _, con := range d.cons { |
There was a problem hiding this comment.
may be should be a ring instead of a list, so on next send u try to use another relay? to balance message sending over multiple relays! Not sure if this is the best approach
There was a problem hiding this comment.
Result of connections used after using ring:
con.url: wss://relay.dev.grid.tf
con.url: wss://relay.02.dev.grid.tf
con.url: wss://relay.dev.grid.tf
con.url: wss://relay.02.dev.grid.tf
con.url: wss://relay.dev.grid.tf
con.url: wss://relay.02.dev.grid.tf
con.url: wss://relay.dev.grid.tf
con.url: wss://relay.02.dev.grid.tf
con.url: wss://relay.dev.grid.tf
con.url: wss://relay.02.dev.grid.tf
|
Note: for the atomic boolean before and after connect, connection tries to send before connection so, It always returns it is busy |
rawdaGastan
force-pushed
the
development_multi_relays_support
branch
2 times, most recently
from
bbd7e29 to
f421d0a
Compare
…n, add atomic bool to watch busy connections
rawdaGastan
force-pushed
the
development_multi_relays_support
branch
from
f421d0a to
c8ee297
Compare
Related Issues