-
Notifications
You must be signed in to change notification settings - Fork 786
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
2216 notify relay when finished #2257
Conversation
|
||
:param server_ip: The IP address of the server to notify. | ||
:param server_port: The port of the server to notify. | ||
:raises OSError: If the disconnect message failed to send. |
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.
Are there other errors that could be raised? ATM, _close_tunnel()
doesn't catch any exceptions. If notify_disconnect()
raises an exception, than self._singleton.unlock()
will never be called, which would prevent new agents from being spawned on the machine.
I think it's fine if this function catches all exceptions and logs them. Either that or _close_tunne()
should catch and log. Either way is fine with me, as long as we're confident that self._singleton.unlock()
will always get called.
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.
Update: I pushed 8858bf9 to ensure that the "singleton" is always unlocked. We should still scrutinize this to make sure we're happy with how errors are being caught. If the cleanup routine changes in the future, uncaught errors here could cause issues.
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.
Yeah, I looked into this but the socket
documentation is pretty vague. From what I can surmise, all socket errors derive from OSError
. I opted to allow the function to raise errors to give the caller the option to decide what to do if the disconnect message fails to send. If we're happy not handling that and just logging it, I can switch to logging it here.
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.
Let's handle it in _close_tunnel()
. Then the caller still decides and it's handled.
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.
This is now updated after rebasing onto the latest 2216-tcp-relay
8858bf9
to
ab7ac76
Compare
2216 notify relay when finished
What does this PR do?
Fixes part of #2216.
Sends the notification upstream to tell the agent's relay that it is disconnecting.
PR Checklist
Was the CHANGELOG.md updated to reflect the changes?Was the documentation framework updated to reflect the changes?Testing Checklist
Added relevant unit tests?Have you successfully tested your changes locally? Elaborate: