BUG: Fix missing "server_close" method on SAMPHubProxy by embray · Pull Request #11391 · astropy/astropy

embray · 2021-03-15T16:52:07Z

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 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.

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.

embray · 2021-03-15T16:53:31Z

github-actions bot added installation and removed testing labels now

but why...?

pllim · 2021-03-15T17:25:21Z

It added installation because of

astropy/.github/labeler.yml

Line 36 in d971b1c

- setup.*

and removed testing because of what I think is a bug in the Action but Microsoft didn't respond to actions/labeler#112 .

pllim

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 .

pllim · 2021-03-15T17:29:49Z

        self._connected = False
        self.lockfile = {}

-    def server_close(self):


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?

No, it's not documented and does not actually do anything useful. Its mere existence is a bug :)

In other words, any attempt to actually call this method will crash.

Ah, okay. Then I guess we can still backport. Thanks for the clarification!

pllim · 2021-03-15T17:32:52Z

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 server_close, maybe it needs one, not sure; Let's wait and see what Tom R says.

pllim

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!

embray · 2021-03-18T15:27:01Z

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 Manually resolved conflicts: setup.cfg

eteq · 2021-03-18T16:13:23Z

I moved this to the 4.0.6 milestone to not block release, but @pllim pointed out it's part of the CI work so I re-milestoned it back to 4.0.5 and backported in 3a3f0da

dhomeier · 2021-03-19T21:48:42Z

I keep seeing (erratic?) failures like this https://github.com/astropy/astropy/pull/11397/checks?check_run_id=2152248020
on TestWebProfile.test_web_profile - is this still related, or another can of worms?

>               warnings.warn(pytest.PytestUnhandledThreadExceptionWarning(msg))
E               pytest.PytestUnhandledThreadExceptionWarning: Exception in thread Thread-275
E               
E               Traceback (most recent call last):
E                 File "/home/runner/work/astropy/astropy/astropy/samp/utils.py", line 62, in __call__
E                   response = function(*args, **kwrds)
E                 File "/opt/hostedtoolcache/Python/3.8.8/x64/lib/python3.8/xmlrpc/client.py", line 1109, in __call__
E                   return self.__send(self.__name, args)
E                 File "/opt/hostedtoolcache/Python/3.8.8/x64/lib/python3.8/xmlrpc/client.py", line 1450, in __request
E                   response = self.__transport.request(
E                 File "/opt/hostedtoolcache/Python/3.8.8/x64/lib/python3.8/xmlrpc/client.py", line 1153, in request
E                   return self.single_request(host, handler, request_body, verbose)
E                 File "/opt/hostedtoolcache/Python/3.8.8/x64/lib/python3.8/xmlrpc/client.py", line 1169, in single_request
E                   return self.parse_response(resp)
E                 File "/opt/hostedtoolcache/Python/3.8.8/x64/lib/python3.8/xmlrpc/client.py", line 1341, in parse_response
E                   return u.close()
E                 File "/opt/hostedtoolcache/Python/3.8.8/x64/lib/python3.8/xmlrpc/client.py", line 655, in close
E                   raise Fault(**self._stack[0])
E               xmlrpc.client.Fault: <Fault 5: 'Private-key 732dec38-88fb-11eb-8e1e-5767653fb47f expired or invalid.'>
E               
E               During handling of the above exception, another exception occurred:
E               
E               Traceback (most recent call last):
E                 File "/opt/hostedtoolcache/Python/3.8.8/x64/lib/python3.8/threading.py", line 932, in _bootstrap_inner
E                   self.run()
E                 File "/opt/hostedtoolcache/Python/3.8.8/x64/lib/python3.8/threading.py", line 870, in run
E                   self._target(*self._args, **self._kwargs)
E                 File "/home/runner/work/astropy/astropy/astropy/samp/tests/web_profile_test_helpers.py", line 163, in _serve_forever
E                   results = self.hub.pull_callbacks(self.get_private_key(), 0)
E                 File "/home/runner/work/astropy/astropy/astropy/samp/tests/web_profile_test_helpers.py", line 90, in pull_callbacks
E                   return self._samp_hub.pullCallbacks(private_key, timeout)
E                 File "/home/runner/work/astropy/astropy/astropy/samp/utils.py", line 64, in __call__
E                   raise SAMPProxyError(exc.faultCode, exc.faultString)
E               astropy.samp.errors.SAMPProxyError: <SAMPProxyError 5: 'Private-key 732dec38-88fb-11eb-8e1e-5767653fb47f expired or invalid.'>

BUG: Fix missing "server_close" method on SAMPHubProxy

Manual backport of #11391 (BUG: Fix missing "server_close" method on SAMPHubProxy)

embray added testing Affects-dev PRs and issues that do not impact an existing Astropy release samp Priority-Medium labels Mar 15, 2021

embray added this to the v4.0.5 milestone Mar 15, 2021

github-actions Bot added installation and removed testing labels Mar 15, 2021

typo in docstring [skip ci]

903393a

pllim added testing and removed installation labels Mar 15, 2021

pllim reviewed Mar 15, 2021

View reviewed changes

pllim requested a review from astrofrog March 15, 2021 17:31

pllim added no-changelog-entry-needed and removed Affects-dev PRs and issues that do not impact an existing Astropy release labels Mar 15, 2021

pllim approved these changes Mar 16, 2021

View reviewed changes

eteq modified the milestones: v4.0.5, v4.0.6 Mar 18, 2021

embray merged commit 55c6ae6 into astropy:master Mar 18, 2021

embray deleted the issue-11167 branch March 18, 2021 15:27

embray added a commit to pllim/astropy that referenced this pull request Mar 18, 2021

Merge pull request astropy#11391 from embray/issue-11167

95f5e29

BUG: Fix missing "server_close" method on SAMPHubProxy

embray mentioned this pull request Mar 18, 2021

TST: Use Actions for LTS #11123

Merged

5 tasks

eteq modified the milestones: v4.0.6, v4.0.5 Mar 18, 2021

embray added a commit to pllim/astropy that referenced this pull request Mar 18, 2021

Merge pull request astropy#11391 from embray/issue-11167

d7fced5

BUG: Fix missing "server_close" method on SAMPHubProxy

eteq pushed a commit that referenced this pull request Mar 18, 2021

Merge pull request #11391 from embray/issue-11167

3a3f0da

BUG: Fix missing "server_close" method on SAMPHubProxy Manually resolved conflicts: setup.cfg

pllim pushed a commit to pllim/astropy that referenced this pull request Mar 24, 2021

Merge pull request astropy#11391 from embray/issue-11167

1af135d

BUG: Fix missing "server_close" method on SAMPHubProxy

pllim mentioned this pull request Mar 24, 2021

Manual backport of #11391 (BUG: Fix missing "server_close" method on SAMPHubProxy) #11419

Merged

eteq added a commit that referenced this pull request Mar 25, 2021

Merge pull request #11419 from pllim/backport-pr11391

cde0cfa

Manual backport of #11391 (BUG: Fix missing "server_close" method on SAMPHubProxy)

This was referenced Mar 30, 2021

TST: SAMPProxyError Private-key expired or invalid #11407

Closed

Fix for #11407 #11447

Merged

Uh oh!

Uh oh!

Conversation

embray commented Mar 15, 2021

Description

Uh oh!

embray commented Mar 15, 2021

Uh oh!

pllim commented Mar 15, 2021

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

pllim Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

embray Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

embray Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

pllim Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

pllim commented Mar 15, 2021

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

embray commented Mar 18, 2021

Uh oh!

eteq commented Mar 18, 2021

Uh oh!

dhomeier commented Mar 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants