[jitsi-dev] [jitsi/jitsi-videobridge] Breaks out of the loop if the SCTP socket early (#226)


#1

Breaks out of the loop if the SCTP socket is closed before trying to receive from the ICE socket.

This potentially avoids an infinite loop, if the ICE socket
implementation is faulty and returns something (e.g. a packet with
length 0) when the socket is closed. More importantly, it makes it
obvious to anyone reading the code that such a scenario is not possible.
You can view, comment on, or merge this pull request online at:

  https://github.com/jitsi/jitsi-videobridge/pull/226

-- Commit Summary --

  * Breaks out of the loop if the SCTP socket is closed before trying to

-- File Changes --

    M src/main/java/org/jitsi/videobridge/SctpConnection.java (6)

-- Patch Links --

https://github.com/jitsi/jitsi-videobridge/pull/226.patch
https://github.com/jitsi/jitsi-videobridge/pull/226.diff

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/226


#2

@@ -1080,6 +1080,9 @@ public void run()
         {
             do
             {
+ if (sctpSocket == null)
+ break;
+

Could I please ask you to write a comment explaining why the check's there? I got the reason from your PR comment but I don't think that's obvious (to me).

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/226/files/daf5bb49cb808b42c69c103b963996f050985616#r60424936


#3

@@ -1118,9 +1121,6 @@ public void run()
                     }
                 }

- if (sctpSocket == null)
- break;
-

I get the reason why the same check is necessary before the receive (after reading your PR comment). But I don't get why it's no longer necessary after the receive. Generally, a receive usually takes significantly long time to return so checks from before may not hold afterwards. In the case that may or may not be true, I'm just saying that it's not obvious to me.

On the other hand, the for loop bellow will repeatedly access sctpSocket so it's not like it will not become null inside the loop (I guess).

Anyway, I suppose I mostly wanted to express my feeling that something's amiss around these null checks outside a loop and accesses to a field in a loop.

It's outside the scope of this PR, of course, but I really feel that this method should be broken down into multiple smaller ones in order to aid reading and understanding.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/226/files/daf5bb49cb808b42c69c103b963996f050985616#r60431306


#4

@@ -1118,9 +1121,6 @@ public void run()
                     }
                 }

- if (sctpSocket == null)
- break;
-

I will close this PR. I agree that we should refactor this anyway, and doing so is on the roadmap, so I'll leave it for now.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/226/files/daf5bb49cb808b42c69c103b963996f050985616#r60435101


#5

Closed #226.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/226#event-634759566


#6

@@ -1118,9 +1121,6 @@ public void run()
                     }
                 }

- if (sctpSocket == null)
- break;
-

I'm sorry.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/226/files/daf5bb49cb808b42c69c103b963996f050985616#r60441761