Safety #294
Safety #294
Conversation
Codecov Report
@@ Coverage Diff @@
## master #294 +/- ##
==========================================
+ Coverage 99.1% 99.24% +0.14%
==========================================
Files 26 26
Lines 3557 3588 +31
Branches 258 257 -1
==========================================
+ Hits 3525 3561 +36
+ Misses 19 15 -4
+ Partials 13 12 -1
Continue to review full report at Codecov.
|
osbrain/agent.py
Outdated
| """ | ||
| Close a socket given its alias and clear its entry from the | ||
| `Agent.socket` dictionary. | ||
| """ | ||
| if linger is None: |
There was a problem hiding this comment.
Why change the linger thing?
I mean, this is what I had in mind a while ago when I was looking at #256, but I don't see how adding linger as a parameter can help this PR at all. (this comment also applies to the changes in .close_all())
There was a problem hiding this comment.
Also, don't you think this would need new tests? Maybe we should open a new issue so we don't forget when we come back to this project in the future.
There was a problem hiding this comment.
As mentioned in my other comment it is to set linger to zero when killing (i.e.: killing should be fast).
This would definitely need more tests. I do not think we need to create a new issue, as there is #255 already.
Anyway I think you are right. I will either add some tests or undo that linger change (and remove the _die() method, which would make little sense then.
osbrain/agent.py
Outdated
| else: | ||
| self._die() | ||
|
|
||
| def _die(self): |
There was a problem hiding this comment.
The _die method seems overly complicated to me. I think it just adds an unnecessary amount of code. Go on with whichever solution you like best, but consider this implementation as well:
def run(self):
...
if self._kill_now:
self.kill()
def kill(self):
self.stop_all_timers()
if self._running:
self._context.term()
else:
self.close_all()
self._pyroDaemon.shutdown()
# And delete the `die` method.If you end up keeping yours, maybe consider renaming _kill_now to _die_now
There was a problem hiding this comment.
Yeah, I agree with you. I forgot to upload one more change though, which perhaps justifies that division.
When cleanly shutting down, we use the default/configured linger, but when killing the agent we do not want to wait, so we want to close all sockets without waiting. In that case I would not add a parameter to the kill() method, as killing should always mean "right now". Hence the reason to create a new internal method.
I just pushed a fixup with that change only. What you think?
I can rename the _kill_now variable as you say. Makes more sense anyway.
There was a problem hiding this comment.
I see what you mean. I don't want to complain too much about this since it makes sense for the most part, so check out my latest revision and then we can move on with this.
osbrain/agent.py
Outdated
|
|
||
| def _die(self): | ||
| def _die(self, linger): |
There was a problem hiding this comment.
Not really necessary, but I think it looks better 😁
Consider changing the method signature to:
def _die(self, linger=None)And then change the end of .run() to self._die()
There was a problem hiding this comment.
Changed signature and renamed attribute.
|
Maybe merge #296 first? That way we can merge this with the tests passing. |
|
@ocaballeror Rebased, squashed and added socket close with linger tests. |
|
Also pushed a new commit to fix: https://travis-ci.org/opensistemas-hub/osbrain/jobs/364043140 I looked for other possible occurrences in our test suite and that seems to be the last one. |
Fixes #287.
Not really proud about the final result, but should definitely be a step forward...