refactor: Load balancer concurrency #4
Conversation
DavidNix
changed the title
| return sl.SendRequestLocked(request) | ||
| } | ||
|
|
||
| // SendRequest ensures there is a connection, sends a request and waits for a response | ||
| func (sl *SignerListenerEndpoint) SendRequestLocked(request privvalproto.Message) (*privvalproto.Message, error) { |
There was a problem hiding this comment.
I removed the SendRequestLocked because I don't think the caller should ever need to use the underlying mutex. That is a recipe for deadlocks.
There was a problem hiding this comment.
recipe for deadlocks with future changes on accident, I agree
There was a problem hiding this comment.
Exactly, for changes in the future where we forget the intricacies.
| lis := <-lb.avail | ||
| defer func() { lb.avail <- lis }() |
There was a problem hiding this comment.
This was the main change.
Pull a listener off the queue (i.e. channel). Then put it back when we're done with it. Line 31 will block if there are no available listeners. Line 32 will never block because the chan buffer is equal to the number of listeners.
|
I'll have a followup PR to handle the case where the SignerListener is currently handling a request. |
When you mix several sync primitives (mutex, wait groups, etc.) that's typically a code smell.
The goal of the load balancer is to choose the first available listener than can serve the request.
This removes the primitives in favor of a single channel that sort of acts like a semaphore.
It also has the advantage of preventing extra goroutines. Previously, on each
SendRequestN goroutines would fire up where N = number of listeners. You could imagine ifSendRequestwere called a lot that could explode goroutines.Now there are 0 extra goroutines.