search

bessey / bt-elixir

Learning Elixir via building a BitTorrent client
3 stars 1 forks source link

Use Dynamic Task Supervisor for the PeerDownloader #1

Open mpope9 opened 4 years ago

mpope9 commented 4 years ago

A bit minor, but I see the PeerDownloader just uses Task.async to spawn a new Task. I think a supervisor added to the top level supervision tree would be useful in restarting a peer in case of a network drop? https://hexdocs.pm/elixir/Task.html#module-dynamically-supervised-tasks

Easy to dynamically add a task: Task.Supervisor.async(Bittorent.PeerSupervisor, your_function/1)

mpope9 commented 4 years ago

Oh I just read the Architecture section. I still think this is a good option, the PeerDownloader isn't really supervising the Tasks, it just dispatches them if I am understanding this correctly.

bessey commented 4 years ago

Did some reworking here this evening, there's now a proper supervision tree. The PeerDownloaders do not yet launch their Tasks under a supervisor, so maybe I can go further, but I've yet to see a time a Task crashes under the latest implementation.

Matt

On Sat, 25 Jan 2020 at 23:51, Matthew Pope notifications@github.com wrote:

Oh I just read the Architecture section. I still think this is a good option, the PeerDownloader isn't really supervising the Tasks, it just dispatches them if I am understanding this correctly.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bessey/bt-elixir/issues/1?email_source=notifications&email_token=AAFM42DTSHNQ4HD755S24ILQ7TGBPA5CNFSM4KLTSLR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ5IGWQ#issuecomment-578454362, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFM42BGQ6QKVM2NHOSQAXDQ7TGBPANCNFSM4KLTSLRQ .