[jitsi-dev] [jitsi/jitsi-videobridge] ice4j VSL (#373)


#1

You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * feat: Adds a property to enable the use of the ice4j virtual socket layer.
  * style: Exports property names as public.
  * style: Moves a static method before field declarations, makes it private.
  * style: Uses camel case for a non-final variable.
  * feat: Adapts to using the ice4j VSL.
  * logs: Logs a message if the ice4j VSL is enabled.
  * feat: Removes the VSL property.
  * cleanup: Renames a method, clean up docs.
  * feat: Handles the newly introduces in ice4j SocketClosedException.
  * cleanup: Removes an obsolete comment.
  * feat: Reduces the dependency to netaddr/nams.
  * feat: Removes tcp-specific functionality from jitsi-videobridge
  * feat: Adapts the ice4j. Simplifies code.
  * docs: Adds a comment.
  * style: Fix a typo.
  * fix: Fix a bug introduces while refactoring.

-- File Changes --

    M src/main/java/org/jitsi/videobridge/Channel.java (2)
    M src/main/java/org/jitsi/videobridge/Endpoint.java (2)
    M src/main/java/org/jitsi/videobridge/IceUdpTransportManager.java (590)
    D src/main/java/org/jitsi/videobridge/JitsiTransportManager.java (144)
    M src/main/java/org/jitsi/videobridge/Main.java (26)
    M src/main/java/org/jitsi/videobridge/RawUdpTransportManager.java (1)
    M src/main/java/org/jitsi/videobridge/RtpChannel.java (25)
    M src/main/java/org/jitsi/videobridge/SctpConnection.java (5)
    M src/main/java/org/jitsi/videobridge/TransportManager.java (22)
    M src/main/java/org/jitsi/videobridge/Videobridge.java (1)

-- Patch Links --

https://github.com/jitsi/jitsi-videobridge/pull/373.patch
https://github.com/jitsi/jitsi-videobridge/pull/373.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/373


#2

bbaldino commented on this pull request.

@@ -1158,8 +1158,11 @@ public void run()

         }
         catch (SocketException ex)
         {
- if (!"Socket closed".equals(ex.getMessage()))
+ if (!"Socket closed".equals(ex.getMessage())

the string comparison seems like a bit of a bummer. is it possible other exceptions (which we don't know the types of) might throw that same error message?

···

--
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/373#pullrequestreview-12521751


#3

bgrozev commented on this pull request.

@@ -1158,8 +1158,11 @@ public void run()

         }
         catch (SocketException ex)
         {
- if (!"Socket closed".equals(ex.getMessage()))
+ if (!"Socket closed".equals(ex.getMessage())

But we are only catching SocketException-s in this block. Am I missing something?

···

--
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/373


#4

bbaldino commented on this pull request.

@@ -1158,8 +1158,11 @@ public void run()

         }
         catch (SocketException ex)
         {
- if (!"Socket closed".equals(ex.getMessage()))
+ if (!"Socket closed".equals(ex.getMessage())

i'm just wondering why we have to do the string comparison and why the 'instanceof' check isn't enough?

···

--
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/373


#5

bgrozev commented on this pull request.

@@ -1158,8 +1158,11 @@ public void run()

         }
         catch (SocketException ex)
         {
- if (!"Socket closed".equals(ex.getMessage()))
+ if (!"Socket closed".equals(ex.getMessage())

The jdk sockets throw a vanilla SocketException with that string, and this is how we've been recognizing it until now, which is awful. The SocketClosedException is just something I added in ice4j, but it is only thrown from MergingDatagramSocket right now. I guess we could make all of the ice4j sockets throw this, and even recognize the string there and re-throw, but I think if we go that way it should be a separate effort.

···

--
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/373


#6

bbaldino commented on this pull request.

@@ -1158,8 +1158,11 @@ public void run()

         }
         catch (SocketException ex)
         {
- if (!"Socket closed".equals(ex.getMessage()))
+ if (!"Socket closed".equals(ex.getMessage())

ok gotcha, sounds like we're stuck then. thanks for the background.

···

--
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/373


#7

@bgrozev pushed 1 commit.

be35e10 mvn: Updates ice4j.

···


You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/373/files/769ab2311804caf60049479d0dca90d6649a62f6..be35e103e29a609a08c5addd5d1836c8a165041c


#8

jenkins ok to test

···

--
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/373#issuecomment-266809987


#9

bbaldino approved this pull request.

lgtm

···

--
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/373#pullrequestreview-12744380


#10

Merged #373.

···

--
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/373#event-892532043