RPC WS refactor #811
Closed
RPC WS refactor #811
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
baum
force-pushed
the
rpc_refactor
branch
12 times, most recently
from
d917f82
to
3149675
Compare
- Use channel/goroutine for sending messages - Rework connect/reconnect logic - Remove WS specific logic from common RPC layer Signed-off-by: Alexander Indenbaum <aindenba@redhat.com>
romayalon
reviewed
Dec 9, 2021
| logrus.Infof("RPC: no connection to %v, reconnect in %v at %v", address, time.Until(*reconn), reconn) | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Where do you handle deleting items from reconnMap?
| var ( | ||
| connMap = make(map[string]*RPCConnWS) | ||
| reconnMap = make(map[string]*time.Time) | ||
| connMapLock = sync.Mutex{} |
There was a problem hiding this comment.
something I was wondering about, do we need another lock for the reconnMap?
| @@ -18,7 +18,7 @@ type RPCConnHTTP struct { | |||
| } | |||
|
|
|||
| // NewRPCConnHTTP returns a new http connection | |||
| func NewRPCConnHTTP(r *RPC, address string) *RPCConnHTTP { | |||
| func NewRPCConnHTTP(r *RPC, address string) RPCConn { | |||
There was a problem hiding this comment.
why not keep using RPCConnHTTP?
Merged
|
Since this PR is changing a very infrastructural module and is not addressing a specific bug, I want to avoid unintended consequences. |
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RPC WS refactor
Description
Abstract
RPCConnWS implements RPCConn interface used by the RPC layer in terms of nhooyr.io/websocket transport. RPC messages are dispatched according to address URL protocol in GetConnection().
States
RPCConnWS is a WebSocket connection that is shared and multiplexed
for all concurrent requests to the same address. An RPCConnWS instance is constructed in a connected state and ready for executing RPC Calls.
nilis returned if connection refused or within reconnect delay. RPCConnWS detects peer disconnect on send and receives paths enhanced by heartbeat Ping goroutine and closes gracefully.Connection, reconnection map
NewRPCConnWS uses connection and reconnection tables mapped by the connection's address URL. The state shared by the construction and closing logic is guarded by the mutex, unlocking is insured by deferring unlock.
IO
Threads
An open connection is served by three goroutines two of those pump messages between the WebSocket and Call and Reply channels, while one is used for WS control ping-pong to improve reaction time to peer disconnect.
Shared state
IO "in-flight" state is tracked using RPCConnWS PendingRequests mapped by the request's ID. Request ID counter increment, the state shared by the IO read/send/close threads is guarded by the connection mutex, unlocking is insured by deferring unlock.