WebSocket Client and V5 RemoteCommand Subprotocol#119157
Conversation
|
/sig api-machinery |
|
/priority important-soon |
|
/cc @aojea |
ae80a42 to
ec34bf5
Compare
|
/retest |
1 similar comment
|
/retest |
|
/cc @jpbetz |
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
There was a problem hiding this comment.
The duration d must be greater than zero; if not, NewTicker will panic. Stop the ticker to release associated resources.
https://pkg.go.dev/time#NewTicker
can the user disable the heartbeat?
can the user set it to zero?
There was a problem hiding this comment.
The heartbeat ping period is a constant (5 seconds) and is not user-configurable. The period will never be zero.
// pingPeriod defines how often a heartbeat "ping" message is sent.
pingPeriod = 5 * time.Second
The reason it is a field in the heartbeat struct is for testing purposes. It allows us to set significantly shorter ping periods for unit tests.
There was a problem hiding this comment.
In case this becomes configurable we should ensure zero disables the heartbeat
|
LGTM some questions that. may be follow ups if they make sense |
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: e63541a9b1e44c41bd81732a42f56e4a220c19c1 |
liggitt
left a comment
There was a problem hiding this comment.
/lgtm
/approve
/hold for one question about the websocket impl matching the spdy impl lack of thread safety
There was a problem hiding this comment.
the returned executor's Stream / StreamWithContext methods are not threadsafe, right? I think the spdy executor has the same issue (due to the coupled upgrader storing state during the connection setup) but can you verify that? would be good to explicitly document that here and on the spdy constructor, but it's fine if that goes into a follow up
There was a problem hiding this comment.
Yes. You are correct that neither the SPDY nor WebSocket Stream/StreamWithContext are thread-safe for a single executor. As you mention, the UpgradeRoundTripper for both store state (the dialed/created connection). SPDY already has some documentation related to this when defining the SpdyRoundTripper struct. Above the connection field is the following comment
type SpdyRoundTripper struct {
...
/* TODO according to http://golang.org/pkg/net/http/#RoundTripper, a RoundTripper
must be safe for use by multiple concurrent goroutines. If this is absolutely
necessary, we could keep a map from http.Request to net.Conn. In practice,
a client will create an http.Client, set the transport to a new insteace of
SpdyRoundTripper, and use it a single time, so this hopefully won't be an issue.
*/
// conn is the underlying network connection to the remote server.
conn net.Conn
I have added documentation about the lack of thread safety to WebSocket.Stream/StreamWithContext
|
/retest unrelated required test failure |
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 4cf19be17ea27119104412468df71489446462a9 |
@liggitt it is on hold and the approve label was not added |
|
/hold cancel |
WebSocketExecutor. The executor is not yet called by any client.doc.goinwsstreampackage describes every version of binary WebSockets RemoteCommand protocols.tools/remotecommandtest coverage from53.8%to75.5%.k8s.io/client-go/tools/remotecommmand/websocket.gohas test coverage:88.3%Example stress test for WebSocket client testing STDIN and STDOUT stream
This loopback test sends 1MB of random data onto the STDIN stream copying the data back to the client onto the STDOUT stream.
Example stress test for WebSocket client sending multiple write streams concurrently (STDIN, TTY Resize)
/kind feature
Helps fix: #89163
NOTE: WebSocket client is implemented with the Gorilla WebSockets library
RoundTripper, because they return an http response which is part of theRoundTripperinterface when other libraries do not.