- 
                Notifications
    You must be signed in to change notification settings 
- Fork 22
Add support for refreshing the sockets config #351
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
base: main
Are you sure you want to change the base?
Conversation
| not Client.ready_to_connect?() -> | ||
| schedule_establish_connection_check() | ||
| {:ok, socket} | ||
|  | ||
| config.connect_wait_for_network -> | 
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.
Is the assumption here that if you ever implement checks to return false on ready_to_connect?, you will skip any wait_for_network check?  If I've traced things right, that seems to be the behavior
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.
That was the original intent, but I can change this, thoughts?
|  | ||
| @impl Slipstream | ||
| def handle_cast({:refresh_config, config}, socket) do | ||
| socket = if(connected?(socket), do: disconnect(socket), else: socket) | 
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.
Slipstream says disconnect/1 is a no-op if it is already disconnected, so could skip the check here if so desired.
Also, if you do disconnect, do you also need to ensure the socket connects again? Since there is a new :auto_connect assigns, I'm feel like refreshing could get you into a state of being disconnected?
There is also the problem on if the Supervisor restarts the child. This only refreshes running state, but the spec stored with NervesHubLink.supervisor will still be from when it was launched. Perhaps doing the refresh outside of the socket would be more appropriate?
Supervisor.terminate_child(NervesHubLink.Supervisor, NervesHubLink.Socket)
Supervisor.delete_child(NervesHubLink.Supervisor, NervesHubLink.Socket)
Supervisor.start_child(NervesHubLink.Supervisor, {NervesHubLink.Socket, config})Though that is aggressive. Whatever route is picked, we should probably document the warnings and precautions of it
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.
Very good catch. I need to think on this a little.
I wonder if Configurator.build() should be called within the Socket.
Thanks @jjcarstens Co-authored-by: Jon Carstens <[email protected]>
The notable changes include:
NervesHubLink.refresh_config()can now be used to refresh the config used by the underlying connection. Coupled withClient.ready_to_connect?(), you can provision a TPM or NervesKey, refresh the config, and then signal to the socket that it can connect.Client.ready_to_connect?()can be used to delay the socket's connection until certain conditions are met, eg, wait for a device's TPM or NervesKey to be initialized. Ifready_to_connect?/0returns false, it will be checked periodically using a 5 second delay.If the application config of
connect: falseis used,NervesHubLink.establish_connection()can be called to manually establish the connection.And
NervesHubLink.disconnect!()can be used to disconnect the socket and disable automatic reconnection.A byproduct of these changes is that the Application supervisor children are always started, which allows for manual connection management.