RSDK-8779: Remove additional cases where signaling server errors override successful PeerConnection attempts. #388

dgottlieb · 2024-11-11T19:34:13Z

Sorry I didn't really take the time to explain the step-by-step. I'm a little impatient with these errors at this point.

…ride successful PeerConnection attempts.

benjirewis

Definitely agree with the changes, some small nits.

rpc/wrtc_client.go

benjirewis

This LGTM % the lint issues.

rpc/wrtc_client.go

dgottlieb · 2024-11-13T16:40:19Z

There's a test unhappy with these changes. It wants to assert on error messages. The test is otherwise valuable, so I'm evaluating options.

such as when the initial offer/answer SDP is not valid

dgottlieb · 2024-11-15T21:49:34Z

logger.go

@@ -25,6 +25,10 @@ type ILogger interface {
 type ZapCompatibleLogger interface {
 	Desugar() *zap.Logger

+	// Not defined: Named(name string) *zap.SugaredLogger


I had a whole episode of adding Named here before realizing why we explicitly need to avoid it.

Why do we explicitly need to avoid Named? Because it creates a new logger that we "lose track of" whereas Sublogger retains the hierarchy needed to allow setting a level on a top-level logger and having it propagate down to subloggers? Or some other interface issue I'm forgetting?

Why do we explicitly need to avoid Named? Because it creates a new logger that we "lose track of" whereas Sublogger retains the hierarchy needed to allow setting a level on a top-level logger and having it propagate down to subloggers?

Correct

dgottlieb · 2024-11-15T21:50:14Z

rpc/dial.go

 					dOpts.authMaterial = dOpts.webrtcOpts.SignalingExternalAuthAuthMaterial
 					dOpts.creds = Credentials{}
 				}
 			}

-			if dOpts.debug {


Felt unnecessary now

dgottlieb · 2024-11-15T21:51:48Z

rpc/dial_test.go

@@ -1150,10 +1150,11 @@ func TestDialMulticastDNS(t *testing.T) {

 	t.Run("fix mdns instance name", func(t *testing.T) {
 		rpcServer, err := NewServer(
-			logger,
+			logger.Named("server"),


Not 100% intentional. I think* these are fine given the logger is golog and this is in tests and not called from RDK.

I think* these are fine given the logger is golog and this is in tests and not called from RDK.

If by fine you mean that we don't need to be using Sublogger here, I would agree. Any reason you needed to create better logger names here, though? Easier to read test output or something?

Any reason you needed to create better logger names here, though? Easier to read test output or something?

"easier to read" is a polite way of putting it. At least in the authenticated subtest, we make 3 separate dials. It was nigh impossible to parse which "base channel/stream" was client and which was server.

dgottlieb · 2024-11-15T21:54:09Z

rpc/wrtc_signaling_server.go

@@ -152,8 +152,37 @@ func HeartbeatsAllowedFromCtx(ctx context.Context) bool {
 	return md[HeartbeatsAllowedMetadataField][0] == "true"
 }

+func (srv *WebRTCSignalingServer) asyncSetOfferError(host string, uuid string, offerErr error) {


This was the big lambda i factored out.

dgottlieb · 2024-11-15T21:54:15Z

rpc/wrtc_signaling_server.go

+	srv.callMu.RUnlock()
+
+	utils.PanicCapturingGo(func() {
+		defer srv.callWorkers.Done()


This changed to a defer

dgottlieb · 2024-11-15T21:56:16Z

rpc/wrtc_signaling_server.go

-	defer func() {
-		var errToSend error
-
-		if callErr == nil {


Notable behavior change is that this callErr == nil block will never result in calling SendOfferError.

As discussed offline, sounds correct to me. Avoiding more of these "exchange happened without error but here's an irrelevant context error for you" situations.

dgottlieb · 2024-11-15T21:56:50Z

rpc/wrtc_signaling_server.go

+		sendCtx, cancel := context.WithTimeout(context.Background(), time.Second*5)
+		defer cancel()
+
+		sendErr := srv.callQueue.SendOfferError(sendCtx, host, uuid, offerErr)


Also referenced in the deleted code: a "context timeout" that's concurrent with a "return nil // the connection has succeeded" should never result in this SendOfferError being invoked.

benjirewis

Pretty much LGTM; one comment about naming.

benjirewis · 2024-11-18T14:51:15Z

logger.go

@@ -25,6 +25,10 @@ type ILogger interface {
 type ZapCompatibleLogger interface {
 	Desugar() *zap.Logger

+	// Not defined: Named(name string) *zap.SugaredLogger


Why do we explicitly need to avoid Named? Because it creates a new logger that we "lose track of" whereas Sublogger retains the hierarchy needed to allow setting a level on a top-level logger and having it propagate down to subloggers? Or some other interface issue I'm forgetting?

benjirewis · 2024-11-18T14:51:58Z

rpc/dial.go

@@ -138,11 +138,11 @@ func dial(
 	if !dOpts.mdnsOptions.Disable && tryLocal && isJustDomain {
 		wg.Add(1)
 		go func(dOpts dialOptions) {
+			mdnsLogger := utils.Sublogger(logger, "mdns")


Appreciate the extra subloggers and fewer guards behind dOpts.debug here; thanks.

benjirewis · 2024-11-18T14:54:00Z

rpc/dial_test.go

@@ -1150,10 +1150,11 @@ func TestDialMulticastDNS(t *testing.T) {

 	t.Run("fix mdns instance name", func(t *testing.T) {
 		rpcServer, err := NewServer(
-			logger,
+			logger.Named("server"),


I think* these are fine given the logger is golog and this is in tests and not called from RDK.

If by fine you mean that we don't need to be using Sublogger here, I would agree. Any reason you needed to create better logger names here, though? Easier to read test output or something?

benjirewis · 2024-11-18T14:57:18Z

rpc/wrtc_signaling_server.go

@@ -152,8 +152,37 @@ func HeartbeatsAllowedFromCtx(ctx context.Context) bool {
 	return md[HeartbeatsAllowedMetadataField][0] == "true"
 }

+func (srv *WebRTCSignalingServer) asyncSetOfferError(host, uuid string, offerErr error) {


I like that this is a separate method, and it's much easier to read than whatever was in that defer for Call before.

Suggested change

func (srv *WebRTCSignalingServer) asyncSetOfferError(host, uuid string, offerErr error) {

func (srv *WebRTCSignalingServer) asyncSendOfferError(host, uuid string, offerErr error) {

Distinction between Set and Send here could be helpful in naming. Set, to me, implies setting some local value representative of error, whereas Send makes me think of SendOfferError.

Definitely a selfish change that I can just revert -- not a lot of utility and is a bit of a lightning rod. Almost all of the value of "publishing" the error is for tests. And specifically the tests that are setting up a failing connection attempt and want to assert we failed for the right reason.

And in those cases, we are using the "in memory" signaling service.

So your intuition on set vs. send was the intention I was aiming to achieve.

benjirewis · 2024-11-18T14:59:47Z

rpc/wrtc_signaling_server.go

-	defer func() {
-		var errToSend error
-
-		if callErr == nil {


As discussed offline, sounds correct to me. Avoiding more of these "exchange happened without error but here's an irrelevant context error for you" situations.

benjirewis · 2024-11-18T15:01:46Z

rpc/wrtc_signaling_server.go

@@ -263,6 +250,8 @@ func (srv *WebRTCSignalingServer) Call(req *webrtcpb.CallRequest, server webrtcp
 				},
 			},
 		}); err != nil {
+			// Don't set an error for the connection attempt. Some candidates may have been
+			// exchanged that can result in a successful connection.


Thanks as always for the in-line comments; totally agree with not sending error here.

dgottlieb requested a review from benjirewis November 11, 2024 19:34

viambot added the safe to test Mark as safe to test label Nov 11, 2024

RSDK-8779: Remove additional cases where signaling server errors over…

60685f8

…ride successful PeerConnection attempts.

dgottlieb force-pushed the RSDK-8779 branch from ea214b4 to 60685f8 Compare November 11, 2024 19:35

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 11, 2024

benjirewis reviewed Nov 12, 2024

View reviewed changes

rpc/wrtc_client.go Outdated Show resolved Hide resolved

rpc/wrtc_client.go Outdated Show resolved Hide resolved

rpc/wrtc_client.go Outdated Show resolved Hide resolved

rpc/wrtc_client.go Outdated Show resolved Hide resolved

check error -- check

e333da9

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 12, 2024

comments -- fix compile

af29afe

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 12, 2024

dgottlieb requested a review from benjirewis November 12, 2024 20:41

benjirewis approved these changes Nov 12, 2024

View reviewed changes

rpc/wrtc_client.go Outdated Show resolved Hide resolved

lint

f49d91d

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 12, 2024

reintroduce propagating errors for truly failing cases

452b5fd

such as when the initial offer/answer SDP is not valid