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
BUG: Fix missing "server_close" method on SAMPHubProxy #11391
Conversation
It's possible this bug has existed for a long time and was just hidden. Another possibility is it appeared at some point during refactoring, but as far as I can tell it's been around a long time. The `SAMPWebClient.hub` attribute is still just a `SAMPHubProxy`; it does not make sense for it to have a `server_close` method because it does not itself run or wrap a server. It does however manage a pool of XML-RPC client connections which is deleted when calling `SAMPHubProxy.disconnect()`. To be on the safe side I added a `ServerProxyPool.shutdown()` method to the proxy pool class which closes the client connections. This is called when shutting down the `SAMPWebClient` server loop. Unlike the normal `SAMPClient` class, the `SAMPWebClient` used only in tests never runs a TCP server of its own, so calling `server_close` anywhere in here does not make sense.
but why...? |
It added Line 36 in d971b1c
and removed |
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.
Thanks for looking into this! Tom R should have a look since I think he ported over this code to astropy
back in the days.
rewrite this code on asyncio
Not sure. I think this sub-package is stuck in purgatory given #9763 .
self._connected = False | ||
self.lockfile = {} | ||
|
||
def server_close(self): |
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.
Can we just delete the method like this and backport to LTS? This is effectively an API change. Will this break downstream? Does anyone actually use this method?
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.
No, it's not documented and does not actually do anything useful. Its mere existence is a bug :)
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.
In other words, any attempt to actually call this method will crash.
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.
Ah, okay. Then I guess we can still backport. Thanks for the clarification!
p.s. I removed the "affects dev" label since this actually affects released code. I added another label to indicate you don't need a change log, though given the deletion of |
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.
Since the CI pass, I am approving this. But I'll leave it open for a bit for @astrofrog to have a look. If you have not heard back by the time this is really needed to unblock your other work, feel free to merge. Thanks!
Since this is a blocker for #11123 I will go ahead and merge, and if @astrofrog has any follow-up suggestions I'll deal with it then. |
BUG: Fix missing "server_close" method on SAMPHubProxy
BUG: Fix missing "server_close" method on SAMPHubProxy
BUG: Fix missing "server_close" method on SAMPHubProxy Manually resolved conflicts: setup.cfg
I keep seeing (erratic?) failures like this https://github.com/astropy/astropy/pull/11397/checks?check_run_id=2152248020
|
BUG: Fix missing "server_close" method on SAMPHubProxy
Manual backport of #11391 (BUG: Fix missing "server_close" method on SAMPHubProxy)
Follow-up to #11167 to try to fix the underlying bug in the
astropy.samp
tests.This is affecting the CI on the 4.0.x branch too so it would be helpful to backport it.
I marked this "affects-dev" since it's not really a user-facing bug, but I can still add a changelog if someone thinks it's necessary.
(As an aside, after going through this code, I wonder, now that we're on Python 3.6+ and even dropping 3.6 on the horizon, if it wouldn't make sense to rewrite this code on asyncio, which might make it clearer. Or maybe not, since wrapping one's head around asyncio is not easy either...)
Description
It's possible this bug has existed for a long time and was just hidden.
Another possibility is it appeared at some point during refactoring, but
as far as I can tell it's been around a long time.
The
SAMPWebClient.hub
attribute is still just aSAMPHubProxy
; itdoes not make sense for it to have a
server_close
method because itdoes not itself run or wrap a server.
It does however manage a pool of XML-RPC client connections which is
deleted when calling
SAMPHubProxy.disconnect()
. To be on the safeside I added a
ServerProxyPool.shutdown()
method to the proxy poolclass which closes the client connections. This is called when shutting
down the
SAMPWebClient
server loop. Unlike the normalSAMPClient
class, the
SAMPWebClient
used only in tests never runs a TCP serverof its own, so calling
server_close
anywhere in here does not makesense.