Bump Halibut so HalibutRuntime is Disposed correctly on Tentacle Shutdown #716
Conversation
| @@ -134,5 +135,19 @@ IPEndPoint GetEndPointToListenOn() | |||
| public void Stop() | |||
| { | |||
| } | |||
|
|
|||
| public void Dispose() | |||
There was a problem hiding this comment.
🤔 This struck me as odd that we have to do this, as Autofac naturally disposes of dependencies given to a class.
I tested this, and this is indeed true. So why did we need to do this for Halibut?
Turns out it's the difference between IDisposable and IAsyncDisposable. The former will get disposed by Autofac, and the latter will not 😑.
Since we recently removed the IDisposable interface from Halibut, this is why it has stopped being disposed of by Autofac.
Knowing all this, it made me wonder:
- What we did here will work. But would it be better or worse to put back the
HalibutRuntime.Disposemethod as an alternative to solving this problem? - Also, we end up double disposing
log, as Autofac also disposes it (I tested it to make sure).
There was a problem hiding this comment.
Also, we end up double disposing log, as Autofac also disposes it (I tested it to make sure).
Ahh yep, good catch, we get given the log so we shouldn't dispose of it
There was a problem hiding this comment.
What we did here will work. But would it be better or worse to put back the HalibutRuntime.Dispose method as an alternative to solving this problem?
Ahh yeh I see, again it's given to us so we shouldn't need to do this. If we bump autofac then it knows how to handle IAsyncDisposable but that's a relatively big change.
We could add Dispose back, or wrap it a little bit to fix the issue here until we are ready to bump autofac
| { | ||
| var startScriptCommand = new LatestStartScriptCommandBuilder() | ||
| .WithScriptBodyForCurrentOs( | ||
| $@"cd ""{clientAndTentacle.RunningTentacle.TentacleExe.DirectoryName}"" |
There was a problem hiding this comment.
Wait, do you get Tentacle to restart by running a command through Tentacle to restart itself?
Wouldn't that interfere with itself? I think I'm missing something... 😁
There was a problem hiding this comment.
This is what Dynamic Workers Service does to Dynamic Workers!!
There was a problem hiding this comment.
It's a hectic mess of things you probably shouldn't really do 😆
nathanwoctopusdeploy
force-pushed
the
sast/fix-tentacle-not-disposing-of-halibutruntime
branch
2 times, most recently
from
bc625a2 to
6c79ffb
Compare
…ks with the autofac container. Add tests to ensure a service can restart itself and that ports are being released
nathanwoctopusdeploy
force-pushed
the
sast/fix-tentacle-not-disposing-of-halibutruntime
branch
from
6c79ffb to
49b0176
Compare
nathanwoctopusdeploy
changed the title
|
This pull request has been linked to Shortcut Story #66309: Async halibut are causing tentacle to free up ports slowly.. |
nathanwoctopusdeploy
deleted the
sast/fix-tentacle-not-disposing-of-halibutruntime
branch
Background
Fix a bug where Tentacle does not dispose of the HalibutRuntime on shutdown.
This can result in the listening port for a listening tentacle not being freed up quickly and stop the tentacle service from being stopped and started quickly.
The HalibutRuntime only had a DisposeAsync method, Dispose has also been added so it works correctly with the version of Autofac used in Tentacle.
Fixes #725
[sc-66309]
How to review this PR
Quality ✔️
Pre-requisites