Add TURN support #30

farao · 2020-06-20T17:52:34Z

If SIGNALTOWER_TURN_SECRET is set, the signaltower add to each
joined_room message a turn_user and turn_password field that allows the
client to use the TURN server for 3 hours. In the following 3h, the
client will always get the same login/token with the same end time back
when it joins another room.

farao · 2020-06-20T18:09:44Z

Needs testing together with palavatv/palava-web#59 and palavatv/palava-client#30.

If SIGNALTOWER_TURN_SECRET is set, the signaltower add to each joined_room message a turn_user and turn_password field that allows the client to use the TURN server for 3 hours. In the following 3h, the client will always get the same login/token with the same end time back when it joins another room.

erikzenker · 2020-06-23T19:37:08Z

README.md

@@ -47,6 +60,8 @@ By default, the websocket is bound to all interfaces (0.0.0.0), you can also bin
 export SIGNALTOWER_LOCALHOST
 ```

+## References
+
 [palava protocol]: https://github.com/palavatv/palava-client/wiki/Protocol


The change in the protocol should be documented.

I thought about making an official release after this and writing it in the CHANGELOG?

Issue to track: palavatv/palava-client#32

erikzenker · 2020-06-23T19:42:07Z

lib/signal_tower/room.ex

+
+    {turn_response, next_turn_timestamp} =
+      if System.get_env("SIGNALTOWER_TURN_SECRET") && last_turn_timestamp < now do
+        next_timestamp = now + 3 * 60 * 60


Is the next_timestamp something like a timeout when the turn session times out. If so I would call it like timeout. If not I need more explanation ^^

yes, it's a timeout, I will rename it

erikzenker · 2020-06-23T19:45:54Z

lib/signal_tower/room.ex

+
+    {turn_response, next_turn_timestamp} =
+      if System.get_env("SIGNALTOWER_TURN_SECRET") && last_turn_timestamp < now do
+        next_timestamp = now + 3 * 60 * 60


Could you use a function from the time module to add three hours to the current time? e.g.: add(now, 3, :hour)

I used that before, but since it's only possible to get the system time as unix time, so I would need to always turn that into elixir time and also carry the turn_timestamp around as elixir time only to change it again to unix time for export - so since both ends are unix time, I'm not sure if it makes sense to make it into elixir time in between only to use the add function here?

I could just imagine that this could be a source of failure since the timestamp has no type based unit. Also the variable name does not provide any information about its unit.

erikzenker · 2020-06-23T19:47:54Z

lib/signal_tower/session.ex

@@ -11,48 +11,48 @@ defmodule SignalTower.Session do
    |> (&Process.register(self(), &1)).()
  end

-  def handle_message(msg, room) do
+  def handle_message(msg, {room, ltt}) do


I would not use ltt but the full version instead. You might know what it means now but some programmer which is new to the code base might have problems to understand what it stands for.

true, I will change that

erikzenker · 2020-06-23T19:54:22Z

test/session_test.exs

@@ -15,7 +15,7 @@ defmodule SessionTest do
            "room_id" => "s-room1",
            "status" => %{user: "0"}
          },
-          nil
+          {nil, 0}


Does it make sense to add a test case were the second parameter is not equal to 0?

yes it does, I will add that as well

erikzenker · 2020-06-23T20:03:04Z

test/room_test.exs

+        {timestamp, ""} = Integer.parse(timestamp_str)
+        assert own_id == id
+        assert System.os_time(:second) < timestamp
+        assert timestamp < System.os_time(:second) + 3 * 60 * 60 + 10


I hope this line will not get flaky in CI

I hope that too :D

For my understanding. The clock is faked in this test right?

You mean System.os_time(:second)? No, that's not faked

erikzenker · 2020-06-23T20:05:16Z

test/room_test.exs

@@ -23,7 +23,7 @@ defmodule RoomTest do

    _user2 =
      spawn_user_no_join(fn ->
-        GenServer.call(room_pid, {:join, self(), %{user: "1"}})
+        GenServer.call(room_pid, {:join, self(), %{user: "1"}, 0})


Mmh after reading through the code I don't understand this new parameter. In the test it is always 0. Does it vary with and without turn?

well we have a per user turn timeout which is part of the state of the user session, so usually it's 0 when the session starts but then it will have the value of the last timout generated

is there a way to make that clearer?

Maybe GenServer's handle_call can have 0 as default value

Since this is really only for the test unclear and not for the actual code, I would leave it. The parameter is clearly named now so that if you look into the handler, it should be clear what it means.

Make the session state a map and document in the websocket init the members.

farao · 2020-06-25T14:39:58Z

Ok, I have:

improved the naming
made the parts of the session state more explicit by putting them into a map and documenting them

I have not (yet)

changed something about the time keeping (unix timestamps vs elixir time structure) - I tend to leave it like it is?
wrote a session test for turn where the parameter is not 0 (it's only one test, would you probably like to add it to see if the parameters are more clear now?)

janlelis · 2020-07-10T14:46:42Z

README.md

@@ -47,6 +60,8 @@ By default, the websocket is bound to all interfaces (0.0.0.0), you can also bin
 export SIGNALTOWER_LOCALHOST
 ```

+## References
+
 [palava protocol]: https://github.com/palavatv/palava-client/wiki/Protocol


Issue to track: palavatv/palava-client#32

janlelis · 2020-07-10T14:54:03Z

lib/signal_tower/room.ex

+
+    {turn_response, next_turn_timeout} =
+      if System.get_env("SIGNALTOWER_TURN_SECRET") && turn_timeout < now do
+        next_timeout = now + 3 * 60 * 60


Could this value (3h) be extracted to its own variable/constant ("no magic numbers": https://en.wikipedia.org/wiki/Magic_number_(programming))

janlelis · 2020-07-10T14:56:55Z

lib/signal_tower/room.ex

+  defp send_joined_room(pid, own_id, members, turn_timeout) do
+    now = System.os_time(:second)
+
+    {turn_response, next_turn_timeout} =


*turn_timeout seems to be a kind of expire_date - do you agree this would make the code more readable?

you're right, I renamed it again, now to turn_token_expiry :-)

👍 Naming is everything 😄

janlelis · 2020-07-10T15:14:05Z

test/room_test.exs

@@ -23,7 +23,7 @@ defmodule RoomTest do

    _user2 =
      spawn_user_no_join(fn ->
-        GenServer.call(room_pid, {:join, self(), %{user: "1"}})
+        GenServer.call(room_pid, {:join, self(), %{user: "1"}, 0})


Maybe GenServer's handle_call can have 0 as default value

erikzenker

Thx for adding the turn support

farao mentioned this pull request Jun 20, 2020

Add TURN support palavatv/palava-web#59

Merged

farao mentioned this pull request Jun 20, 2020

Add TURN support palavatv/palava-client#30

Merged

erikzenker reviewed Jun 23, 2020

View reviewed changes

farao force-pushed the turn branch from 4095f7f to 3a78b38 Compare June 25, 2020 14:33

Improve readibility of session state

5c570f0

Make the session state a map and document in the websocket init the members.

farao force-pushed the turn branch from 3a78b38 to 5c570f0 Compare June 25, 2020 14:37

janlelis mentioned this pull request Jul 8, 2020

Testing on Android (Chrome) palavatv/palava-web#17

Open

4 tasks

janlelis reviewed Jul 10, 2020

View reviewed changes

Rename turn_timout to *expiry, make its validity_period a constant

a83d762

erikzenker approved these changes Jul 11, 2020

View reviewed changes

Refactor turn tests, add session turn test

45ad2d5

farao force-pushed the turn branch from 5df4c05 to 45ad2d5 Compare July 12, 2020 14:27

farao merged commit 3230190 into master Jul 23, 2020

farao deleted the turn branch November 9, 2020 22:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TURN support #30

Add TURN support #30

farao commented Jun 20, 2020

farao commented Jun 20, 2020

erikzenker Jun 23, 2020

farao Jun 25, 2020

janlelis Jul 10, 2020

erikzenker Jun 23, 2020

farao Jun 25, 2020

erikzenker Jun 23, 2020

farao Jun 25, 2020

erikzenker Jul 11, 2020

erikzenker Jun 23, 2020

farao Jun 25, 2020

erikzenker Jun 23, 2020

farao Jun 25, 2020

erikzenker Jun 23, 2020

farao Jun 25, 2020

erikzenker Jul 11, 2020

farao Jul 23, 2020

erikzenker Jun 23, 2020

farao Jun 25, 2020

farao Jun 25, 2020

janlelis Jul 10, 2020

farao Jul 11, 2020

farao commented Jun 25, 2020

janlelis Jul 10, 2020

janlelis Jul 10, 2020

farao Jul 11, 2020

janlelis Jul 10, 2020

farao Jul 11, 2020

erikzenker Jul 11, 2020

janlelis Jul 10, 2020

erikzenker left a comment

Add TURN support #30

Add TURN support #30

Conversation

farao commented Jun 20, 2020

farao commented Jun 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

farao commented Jun 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikzenker left a comment

Choose a reason for hiding this comment