multi: attempt to fix issue of high number of TCP connections by explicitly closing connections and implementing an idle timer #719
Conversation
|
PTAL! |
Roasbeef
force-pushed
the
tcp-connection-issue
branch
2 times, most recently
from
ed9b703 to
24eacd6
Compare
proof/courier.go
Outdated
| @@ -467,6 +478,8 @@ func (h *HashMailBox) RecvAck(ctx context.Context, sid streamID) error { | |||
|
|
|||
| // CleanUp atempts to tear down the mailbox as specified by the passed sid. | |||
| func (h *HashMailBox) CleanUp(ctx context.Context, sid streamID) error { | |||
| defer h.rawConn.Close() | |||
There was a problem hiding this comment.
Non-blocking: I guess my request was less about actually logging the error but rather logging it instead of having it as a return value in the interface. Because swallowing the error here does show up as a code smell issue in my IDE (and will probably be flagged by the linter if we decide to turn on some of the rules we have in lnd).
There was a problem hiding this comment.
Maybe over-engineering:
In such use-cases we could try returning a named variable
func (h *HashMailBox) CleanUp(ctx context.Context, sid streamID) (err error) {and assign to err the error we want to return within the body.
Then our defer would look like this
defer func(){
if err != nil {
err = h.rawConn.Close()
}
}()There was a problem hiding this comment.
Or yeah we could just omit returning error. Do we care about consistency on the Close func signature?
proof/courier.go
Outdated
| @@ -467,6 +478,8 @@ func (h *HashMailBox) RecvAck(ctx context.Context, sid streamID) error { | |||
|
|
|||
| // CleanUp atempts to tear down the mailbox as specified by the passed sid. | |||
| func (h *HashMailBox) CleanUp(ctx context.Context, sid streamID) error { | |||
| defer h.rawConn.Close() | |||
There was a problem hiding this comment.
Maybe over-engineering:
In such use-cases we could try returning a named variable
func (h *HashMailBox) CleanUp(ctx context.Context, sid streamID) (err error) {and assign to err the error we want to return within the body.
Then our defer would look like this
defer func(){
if err != nil {
err = h.rawConn.Close()
}
}()
proof/courier.go
Outdated
| @@ -467,6 +478,8 @@ func (h *HashMailBox) RecvAck(ctx context.Context, sid streamID) error { | |||
|
|
|||
| // CleanUp atempts to tear down the mailbox as specified by the passed sid. | |||
| func (h *HashMailBox) CleanUp(ctx context.Context, sid streamID) error { | |||
| defer h.rawConn.Close() | |||
There was a problem hiding this comment.
Or yeah we could just omit returning error. Do we care about consistency on the Close func signature?
|
itest seems to be failing consistently, will dig in. |
proof/courier.go
Outdated
| @@ -467,6 +478,8 @@ func (h *HashMailBox) RecvAck(ctx context.Context, sid streamID) error { | |||
|
|
|||
| // CleanUp atempts to tear down the mailbox as specified by the passed sid. | |||
| func (h *HashMailBox) CleanUp(ctx context.Context, sid streamID) error { | |||
| defer h.rawConn.Close() | |||
There was a problem hiding this comment.
We need to move this h.rawConn.Close() out of the CleanUp() method, since that is being called for two mailboxes (sender+receiver). So it needs to be done in DeliverProof as a defer before cleaning up the mailboxes.
Roasbeef
force-pushed
the
tcp-connection-issue
branch
from
24eacd6 to
de7f577
Compare
… syncers In this commit, we attempt to fix a TCP connection leak by explicitly closing the gRPC connections we create once we're done with the relevant gRPC client. Otherwise, we'll end up making a new connection for each new asset to be pushed, which can add up. In the future, we should also look into the server-side keep alive options.
This is similar to the prior commit: we add a new method to allow a caller to close down a courier once they're done with it. This ensures that we'll always release the resources once we're done with them.
In this commit, we set a gRPC param that controls how long a connection can be idle for. The goal here is to prune the amount of open TCP connections on an active/popular universe server. According to the docs: > Idleness duration is defined since the most recent time the number of outstanding RPCs became zero or the connection establishment.
Roasbeef
force-pushed
the
tcp-connection-issue
branch
from
de7f577 to
b61d6fb
Compare
On the instance we've run, we've been seeing a very high number of active TCP connections at any given time. After tweaking some Aperture related settings, it seems that the issue is with our own gRPC server, and the way we make outbound connections.
This PR implements to fixes to attempt to mitigate this issue: