New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allocate reserved relay address #39
Conversation
It's not possible to start a worker from outside the dispatcher without the receiver's client socket. Since client_info is already passed around, it's the logical place to add the socket to. This way we're gonna be able to start a worker inside the allocate request handler code.
client_info.port is port on which we receive datagrams from the client. Therefore, we can't use this client_info instance to create a reserved worker for the new allocation, as the client will connect from different port. We have to rely on dispatcher to create the new worker process, but at the same time store the reservation somewhere, so that it can be picked up by that new worker when it reads the RESERVATION-TOKEN attribute coming with an allocate request.
This dramatically reduces the overhead (approx. 3x less space consumed per reservation).
Until now the log was tied to the server's listening port. This does not work well with the test helpers we have in place, and is not conformant with the RFC: > [the client] can use a different client IP address and > port, a different transport protocol, and even different server IP > address and port [when requesting an allocation with a RESERVATION-TOKEN]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two tiny issues with code formatting. Overall, it looks great 👍
Logger.warn(:max_retries) | ||
{:error, ErrorCode.new(:insufficient_capacity)} | ||
end | ||
defp open_this_relay( params, state, server) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a space before first param.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. It looked better when the signature was:
defp open_this_relay(_params, %{retries: r}, _server) when r < 0, do: ...
defp open_this_relay( params, state, server) do ...
lib/fennec/turn/reservation.ex
Outdated
socket: socket} | ||
end | ||
|
||
@doc false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need for @doc false
if there is @moduledoc false
.
Can we merge this and work on reservation expiration as a separate issue? I wouldn't want to rebase this if anything unexpected happens on master. |
This might still need some polish / refactoring, but the happy path finally works, i.e. full support for EVEN-PORT and RESERVATION-TOKEN (both issuing by the server to mark a newly created reservation and consuming when sent by a client, to use a previously reserved port).
Open TODOs (only reservation expiration is crucial feature-wise):
This addresses #22, #23.