Simple TURN client #67

arkgil · 2017-03-28T13:23:21Z

Extended client process to issue allocate and refresh requests and react to responses. It also includes retries when stale nonce or unathorized errors are returned from the server. Also moved STUN/TURN processing to Jerboa.Client.Protocol module.

mentels · 2017-03-30T13:32:11Z

lib/jerboa/client.ex

  def bind(client) do
-    GenServer.call(client, :bind)
+    GenServer.call(client, :bind, 2 * timeout())


2 * timeout() doesn't convey any idea "why" you're doing that.

To be honest, I don't know why it it 2*. What do you recommend, leave timeout() or go with the default GenServer timeout? If we go with the default, it doesn't make much sense to configure timeout at all.

If there's no reason at all with changing the timeouts I would get rid of it entirrely.

mentels · 2017-03-30T13:32:57Z

lib/jerboa/client/protocol.ex

+  @moduledoc false
+
+  ## Pure functions which construct requests or indications
+  ## and processes responses


process not processes

mentels · 2017-03-30T13:39:40Z

lib/jerboa/client/protocol.ex

+      {:ok, new_state} ->
+        {{:ok, new_state.mapped_address}, empty_transaction(new_state)}
+      error ->
+      {error, state}


wrong alignment

mentels · 2017-03-30T13:41:53Z

lib/jerboa/client/protocol.ex

+  end
+
+  @spec eval_bind_resp(Worker.state)
+  :: {{:ok, mapped_address :: Client.address}, Worker.state}


I don't like that the :ok or :error are in the inner tuple - is there any reason for that?

This results in the following code: {{:error, :bad_response}, worker_state} = Protocol.eval_bind_resp(..)

Yes, the inner tuple is the result returned to the caller of Jerboa.Client.bind/allocate/refresh..

Do we need to pass up what was returned from the Jerboa.Client.bind/allocate/refresh..?

I mean, when someone calls Jerboa.Client.bind it will internally call Protocol.eval_bind_resp, which returns a two elemenet tuple (from the Client module point of view): {result, new_state} - this way we just extract these two values, and return {:reply, result, new_state} from the handle_call/3 callback.

mentels · 2017-03-30T13:50:49Z

lib/jerboa/client/protocol.ex

+    |> Params.put_attr(%Nonce{value: state.nonce})
+  end
+
+  require Logger


require in the middle of a file?

Missing line from debugging, going to remove that.

mentels · 2017-03-30T13:52:22Z

lib/jerboa/client/worker.ex

+      |> Protocol.eval_bind_resp()
+    {:reply, result, new_state}
+  end
+  def handle_call(:allocate, _, state) do


hmhm that function could be better.. to many levels..

Hm, probably with would make this prettier.

Unfortunately no, I can't solve this using with, because I won't be able to return new_state from one of the else clauses. To be honest, I have no idea how to refactor it at the moment.

if state.relayed_address do {:reply, {:ok, state.relayed_address}, state} else {result, new_state} = request_allocation(state) {:reply, result, new_state} end

why not that way and updating the timer in the request_allocation/1.

Another alternative I can spot is:

if state.relayed_address do {:reply, {:ok, state.relayed_address}, state} else {result, new_state} = request_allocation(state) {:reply, result, maybe_update_lifetime_timer(result, new_state)} end

mentels · 2017-03-30T13:54:10Z

lib/jerboa/client/worker.ex

+    socket = socket(state)
+    {address, port} = server(state)
+    :ok = UDP.send(socket, address, port, req)
+    {:ok, {^address, ^port, response}} = UDP.recv(socket, 0, timeout())


Everytime you need timeout you take it our from the App env? no no no, that's not that good idea.. why not putting it into the server's state?

I have thought about it too. Is it that much overhead of calling application's env? IIRC these are just ETS tables. I know that when we issue many such calls it'd become the problem. I'm gonna fix that.

That's true - it shouldn't be that bad.

mentels · 2017-03-30T13:54:55Z

lib/jerboa/client/worker.ex

+      |> Protocol.eval_allocate_resp()
+    case result do
+      :retry ->
+        request_allocation(new_state)


Infinite loop if we fail to perform an allocation?

What do you think about introducing a retries counter? So that we fail after a certain number of tries. The call to client process would probably fail due to a timeout, but this will ensure that process doesn't get stuck in this loop.

Another reasonable solution would be to fail just after one retry. We retry a request when we get initial unauthorized response, which is completely fine, but subsequent requests shouldn't result in such response.

We also retry when the response is stale_nonce, in which case the next request should succeed. If it doesn't there may be two reasons:

the server is dumb/misconfigured/non-compliant with RFC, and just returns stale_nonce because something bad happens - in this case it is totally fine to not retry

the server has really small nonce expiration time, and due to network conditions our request arrived after the new nonce has expired - I don't think it is likely to happen (it wouldn't be reasonable from the server side) but we can't assume it won't happen at some point. In this case a next retry would be justified, but we can just return stale_nonce error to the caller and assume that he'll retry manually.

What do you think is the most reasonable solution here?

I think that we need to distinguish situations when we want to retry because there's no response (cases when we suspect network failure) from the cases where according to the protocol we could retry.

In the first case the counter looks ok to me (providing the retires are at least logged).

Yes, I think logging is what we should add to the client before even proceeding with next features.

mentels · 2017-03-30T13:55:31Z

lib/jerboa/client/worker.ex

+      |> Protocol.eval_refresh_resp()
+    case result do
+      :retry ->
+        request_refresh(new_state)


Initinite loop?

The same comment as above applies, but here we only retry on stale_nonce.

rslota · 2017-03-31T11:21:35Z

@arkgil Could you please rebase this branch onto master right after #82 is merged?

arkgil · 2017-03-31T11:24:47Z

Yes, that shouldn't be a problem. I don't think the client code will change at all

Use Params API, disable timeouts on indications, use Format.decode! to crash on undecodable messages, store mapped address in client's state.

Until we run Jerboa in real world, we can't assume what timeout is sufficient.

Default retries count for requests is 1.

arkgil · 2017-03-31T13:26:53Z

I think that last thing we need here is some kind of debug logging, and after that we can merge. It has already became too big.

arkgil · 2017-04-03T14:03:57Z

I think it's ready to be merged (unless @rslota or @erszcz want to check it).

rslota · 2017-04-03T14:40:43Z

I think it's ready to be merged (unless @rslota or @erszcz want to check it).

Looks good to me.

arkgil · 2017-04-04T07:51:00Z

I spotted a bug, need a bit more work on that.

jerboabot · 2017-04-04T08:07:17Z

Ebert has finished reviewing this Pull Request and has found:

4 possible new issues (including those that may have been commented here).

But beware that this branch is 4 commits behind the esl:master branch, and a review of an up to date branch would produce more accurate results.

You can see more details about this review at https://ebertapp.io/github/esl/jerboa/pulls/67.

arkgil · 2017-04-04T08:33:53Z

Alright, now everything is fine. Please merge it if you don't see any more issues 🙂

rslota · 2017-04-04T08:44:02Z

Still waiting for @mentels to click "Approve" :)

arkgil mentioned this pull request Mar 28, 2017

Introduce High Level Validation #19

Open

arkgil assigned arkgil and mentels and unassigned arkgil Mar 28, 2017

arkgil mentioned this pull request Mar 30, 2017

Implement Allocate and Refresh behaviour in TURN client #75

Closed

mentels suggested changes Mar 30, 2017

View reviewed changes

arkgil assigned arkgil and unassigned mentels Mar 31, 2017

Arkadiusz Gil added 10 commits March 31, 2017 14:13

Clean up Client.Worker module

62aad21

Use Params API, disable timeouts on indications, use Format.decode! to crash on undecodable messages, store mapped address in client's state.

Move STUN processing from Worker module

3471c43

Implement TURN allocation mechanism

6ec2de3

Refactor common operations into functions

930c269

Handle allocation expiration

b499148

Implement allocation refreshes

7c5e029

Amend typespecs and docs

bd3f477

Remove timeout configuration

121e402

Until we run Jerboa in real world, we can't assume what timeout is sufficient.

Fix minor style issues

6c2c308

Introduce retries counter

fb16414

Default retries count for requests is 1.

arkgil force-pushed the simple-turn-client branch from e59e5aa to fb16414 Compare March 31, 2017 12:15

Arkadiusz Gil added 2 commits April 3, 2017 14:34

[skip ci] Log progress messages from client process

239b207

Update documentation with TURN operations

6244f25

rslota mentioned this pull request Apr 4, 2017

Update jerboa ref esl/MongooseICE#20

Closed

Fix a bug with allocations timer not being set

b215454

mentels approved these changes Apr 4, 2017

View reviewed changes

mentels merged commit d8e6a0d into master Apr 4, 2017

arkgil deleted the simple-turn-client branch April 25, 2017 08:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simple TURN client #67

Simple TURN client #67

arkgil commented Mar 28, 2017

mentels Mar 30, 2017

arkgil Mar 31, 2017

mentels Mar 31, 2017

mentels Mar 30, 2017

mentels Mar 30, 2017

mentels Mar 30, 2017

arkgil Mar 31, 2017

mentels Mar 31, 2017

arkgil Mar 31, 2017

mentels Mar 31, 2017

mentels Mar 30, 2017

arkgil Mar 31, 2017

mentels Mar 30, 2017

arkgil Mar 31, 2017

arkgil Mar 31, 2017

mentels Mar 31, 2017

mentels Mar 30, 2017

arkgil Mar 31, 2017

mentels Mar 31, 2017

mentels Mar 30, 2017

arkgil Mar 31, 2017

mentels Mar 31, 2017

arkgil Mar 31, 2017

mentels Mar 30, 2017

arkgil Mar 31, 2017

rslota commented Mar 31, 2017

arkgil commented Mar 31, 2017

arkgil commented Mar 31, 2017

arkgil commented Apr 3, 2017

rslota commented Apr 3, 2017

arkgil commented Apr 4, 2017

jerboabot commented Apr 4, 2017

arkgil commented Apr 4, 2017

rslota commented Apr 4, 2017

Simple TURN client #67

Simple TURN client #67

Conversation

arkgil commented Mar 28, 2017

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

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

rslota commented Mar 31, 2017

arkgil commented Mar 31, 2017

arkgil commented Mar 31, 2017

arkgil commented Apr 3, 2017

rslota commented Apr 3, 2017

arkgil commented Apr 4, 2017

jerboabot commented Apr 4, 2017

arkgil commented Apr 4, 2017

rslota commented Apr 4, 2017