Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RSDK-8293] Pass in Connection to Answerer in WebRTC Server #410

Merged
merged 19 commits into from
Feb 5, 2025
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions rpc/dial_options.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ type dialOptions struct {
// interceptors
unaryInterceptor grpc.UnaryClientInterceptor
streamInterceptor grpc.StreamClientInterceptor

// conn can be used to force the webrtcSignalingAnswerer to use a preexisting connection instead of dialing and managing its own.
cheukt marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// conn can be used to force the webrtcSignalingAnswerer to use a preexisting connection instead of dialing and managing its own.
// signalingConn can be used to force the webrtcSignalingAnswerer to use a preexisting connection instead of dialing and managing its own.

conn ClientConn
}

// DialMulticastDNSOptions dictate any special settings to apply while dialing via mDNS.
Expand Down Expand Up @@ -280,3 +283,10 @@ func WithForceDirectGRPC() DialOption {
o.disableDirect = false
})
}

// WithConn provides a preexisting connection to use. This option forces the webrtcSignalingAnswerer to not dial or manage a connection.
func WithConn(conn ClientConn) DialOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe ExternalSignalerConn? we should be clear that this would be used for the external signaler answerer

return newFuncDialOption(func(o *dialOptions) {
o.conn = conn
})
}
24 changes: 18 additions & 6 deletions rpc/wrtc_signaling_answerer.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,9 @@ type webrtcSignalingAnswerer struct {
// conn is used to share the direct gRPC connection used by the answerer workers. As direct gRPC connections
// reconnect on their own, custom reconnect logic is not needed. However, keepalives are necessary for the connection
// to realize it's been disconnected quickly and start reconnecting.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update comment here

connMu sync.Mutex
conn ClientConn
connMu sync.Mutex
conn ClientConn
sharedConn bool

logger utils.ZapCompatibleLogger
}
Expand All @@ -61,8 +62,12 @@ func newWebRTCSignalingAnswerer(
dialOptsCopy := make([]DialOption, len(dialOpts))
copy(dialOptsCopy, dialOpts)
dialOptsCopy = append(dialOptsCopy, WithWebRTCOptions(DialWebRTCOptions{Disable: true}))
options := &dialOptions{}
for _, opt := range dialOptsCopy {
opt.apply(options)
}
bgWorkers := utils.NewBackgroundStoppableWorkers()
return &webrtcSignalingAnswerer{
ans := &webrtcSignalingAnswerer{
address: address,
hosts: hosts,
server: server,
Expand All @@ -71,6 +76,11 @@ func newWebRTCSignalingAnswerer(
bgWorkers: bgWorkers,
logger: logger,
}
if options.conn != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the internal signaler answer shouldn't be using the same connection - it is instantiated here

goutils/rpc/server.go

Lines 659 to 665 in cf6c56e

server.webrtcAnswerers = append(server.webrtcAnswerers, newWebRTCSignalingAnswerer(
address,
internalSignalingHosts,
server.webrtcServer,
answererDialOpts,
config,
utils.Sublogger(logger, "signaler.internal"),

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't require any change does it, since its instantiation has its own fresh set of options passed?

ans.conn = options.conn
ans.sharedConn = true
}
return ans
}

const (
Expand Down Expand Up @@ -260,9 +270,11 @@ func (ans *webrtcSignalingAnswerer) Stop() {
ans.connMu.Lock()
defer ans.connMu.Unlock()
if ans.conn != nil {
err := ans.conn.Close()
if isNetworkError(err) {
ans.logger.Errorw("error closing signaling connection", "error", err)
if !ans.sharedConn {
err := ans.conn.Close()
if isNetworkError(err) {
ans.logger.Errorw("error closing signaling connection", "error", err)
}
}
ans.conn = nil
}
Expand Down
Loading