Fix most ResourceWarning in the test suite
https://docs.python.org/dev/whatsnew/3.2.html python3 warns about not unclosed files and sockets.
Use context managers with files to close them.
Use context manager to close socket to supervisor on python 3. On python 2 these sockets remain not closed as before.
There are a few warnings left, but output starts to be readable again.
The changes were merged into master. The source branch has been removed.
1153 1155 # updating promises state, no need to raise here 1154 1156 pass 1155 1157 raise e 1156 else: 1158 finally:
So the initialization of
partition_file_handlershould be done just before the
good point, it was not so far from the
try:but I'll reorganize this to make it just before, maybe with a comment.
I reorganized a bit as suggested.
@jm started a discussion on an outdated diffslapos/grid/svcbackend.py
50 """ 47 51 supervisor_transport = xmlrpc.SupervisorTransport('', '', 48 52 'unix://' + socket) 49 53 server_proxy = xmlrpclib.ServerProxy('http://127.0.0.1', 50 54 supervisor_transport) 51 return getattr(server_proxy, 'supervisor') 55 56 # python3's xmlrpc is a closing context manager, python2 is not and it does 57 # not seem possible to close the transport. 58 # on python2, make it a do-nothing context manager so that we have a 59 # normalized API of always using a context manager 60 if sys.version_info.major == 2: 61 @contextlib.contextmanager 62 def _getSupervisorRPC(): 63 yield server_proxy 64 return _getSupervisorRPC()
Where do you use _getSupervisorRPC ?
it's only internal here, to return a context manager.... but all this is not OK, I should have set WIP flag on this MR, I wanted to investigate this a bit more, also maybe there's a better way.
My understanding so far: On python3,
xmlrpclib.ServerProxyis a context manager which closes the connection, but on python 2 it's not. It has some magic for
closemethod, but it does not seem to work, this might be a bug (
__sendbut the magic uses
__call__), or maybe we are supposed to use
On python2, if we use it as a context manager, the magic seem to call
__exit__methods via XML-RPC (because of the magic
__getattr__). I need to take a closer look into this.
That does not answer my question.
Anyway, don't try to return the Python3 context manager to the caller: make getSupervisorRPC a contextmanager so you can yield
server_proxy.supervisor. Internally, you could still use the Python3 context manager if you want.
Sorry if that was not clear. But the patch is anyway still not clear and there's not much to discuss except "it's wrong".
getSupervisorRPCwas used in several places like this:
supervisornamespace. The API is described in http://supervisord.org/api.html . There's also a
systemnamespace, but as we can see at the end of this doc it's only for introspection, so we don't care and that's why this method returned
getattr(server_proxy, 'supervisor'). It was wrong to change this to return
supervisornamespace, I think this is one thing you suggest, I'll revert this part.
xmlrpclib.ServerProxy, which is on python3 is a context manager which close the sockets. So I was thinking to change so that on python 3, we can do:
with getSupervisorRPC() as supervisor: supervisor.supervisor.getStatus()
and the socket will be closed when exiting the context manager.
The same code on python2 caused problems, because of the magic getattr in
xmlrpclib.ServerProxy, it will call
__enter__as an xml rpc method. That's why I tried to make a contextmanager doing nothing on python2, so that the caller always use a context manager.
_getSupervisorRPCwas an internal function used just here in python2, to return always a context manager.
I tried a bit more and what's almost correct is to use this:
@contextlib.contextmanager def getSupervisorRPC(socket): """Get a supervisor XML-RPC connection. Use in a context manager for proper closing of sockets. """ supervisor_transport = xmlrpc.SupervisorTransport('', '', 'unix://' + socket) server_proxy = xmlrpclib.ServerProxy('http://127.0.0.1', supervisor_transport) try: yield server_proxy.supervisor finally: server_proxy("close")()
It's not correct because this
xmrpc.SupervisorTransportdoes not implement
closeand it's not compatible with the
closeimplementation from parent class.:
File "/opt/slapgrid/b0bcd9831b23f334bc85e4a193c748a0/parts/python2.7/lib/python2.7/xmlrpclib.py", line 1590, in __close self.__transport.close() File "/opt/slapgrid/b0bcd9831b23f334bc85e4a193c748a0/parts/python2.7/lib/python2.7/xmlrpclib.py", line 1397, in close host, connection = self._connection AttributeError: SupervisorTransport instance has no attribute '_connection'
I reported in https://github.com/Supervisor/supervisor/issues/1184 and suggested a patch, but it will take some time.
Now I'm wondering if we really need to close the sockets and re-open a connection every time, maybe
getSupervisorRPCcould be changed to use a singleton
ServerProxyinstance which would connection open and reuse the same connection each time.
The fix for https://github.com/Supervisor/supervisor/issues/1184 landed on supervisor's master branch and we install it from git on python 3, so we can fix for python 3.
I'm using this way:
@contextlib.contextmanager def getSupervisorRPC(socket): """Get a supervisor XML-RPC connection. Use in a context manager for proper closing of sockets. """ supervisor_transport = xmlrpc.SupervisorTransport('', '', 'unix://' + socket) server_proxy = xmlrpclib.ServerProxy('http://127.0.0.1', supervisor_transport) # python3's xmlrpc is a closing context manager, python2 is not and cannot be # just used as a context manager as it would call __enter__ and __exit__ on # XML-RPC. if sys.version_info.major == 2: yield server_proxy.supervisor else: with server_proxy as s: yield s.supervisor
which closes correctly on python 3 and does not close python 2 (same as before).
ah and about this idea of reusing the same
ServerProxyinstance, I did a very quick benchmark and it seems the improvement would not be so significant.
closing and reopening connection each time:
reusing the same connection:
it's of course a bit faster, but I don't think it would make a difference.
The changes from this MR are just about explicitly closing the socket earlier (instead of waiting for the implicit close in
__del__), there's no regression and I abandoned this idea.
@jm started a discussion on an outdated diffslapos/grid/watchdog.py
174 176 partition_home_path, 175 177 COMPUTER_PARTITION_LATEST_BANG_TIMESTAMP_FILENAME 176 178 ) 179 timestamp = '0' 177 180 if os.path.exists(partition_timestamp_file_path): 178 timestamp = open(partition_timestamp_file_path, 'r').read() 179 else: 180 timestamp = '0'
marked as a Work In ProgressToggle commit list
added 28 commits
Toggle commit list
dc870e25...812d00c4 - 25 commits from branch
- 0998e7f1 - *: explictly close file descriptors
- 79a91fb4 - grid,manager: close xmlrpc.ServerProxy sockets
- 35de3ee1 - slapgrid: explicitly close partition file logger for instanciation
- dc870e25...812d00c4 - 25 commits from branch
unmarked as a Work In ProgressToggle commit list
test output on python 3 only have a few resource warning left:
/srv/slapgrid/slappart3/srv/testnode/byq/inst/test0-0/parts/slapos.core/slapos/grid/promise/generic.py:159: ResourceWarning: unclosed file <_io.TextIOWrapper name='/tmp/tmp3lct9cod/log/my_promise.log' mode='a' encoding='UTF-8'> self._configureLogger()Promise plugins uses logging but does not explicitly close. I don't see how to fix the warning without refactoring too much.
/srv/slapgrid/slappart3/srv/testnode/byq/inst/test0-0/parts/slapos.core/slapos/grid/SlapObject.py:647: ResourceWarning: unclosed file <_io.BufferedReader name=12> debug=self.buildout_debug)
/srv/slapgrid/slappart3/srv/testnode/byq/inst/test0-0/parts/slapos.core/slapos/grid/slapgrid.py:1293: ResourceWarning: unclosed file <_io.BufferedReader name=12> self.logger.exception('Problem while reporting error, continuing:')
/srv/slapgrid/slappart3/srv/testnode/byq/inst/test0-0/parts/slapos.core/slapos/grid/slapgrid.py:605: ResourceWarning: unclosed file <_io.BufferedReader name=12> self.logger.exception('Problem while reporting error, continuing:')
/srv/slapgrid/slappart3/srv/testnode/byq/soft/cd8a98ea76df212f1c5fa8b21fddd3aa/parts/python3.5/lib/python3.5/unittest/case.py:601: ResourceWarning: unclosed file <_io.BufferedReader name=7> testMethod()
These 3, I don't know... It might come from
SlapPopen. It's now readable, so I guess we can merge this first and improve more later.
anybody still see issues with this ?
[edit: this was grammatically wrong]
Not me :)
great let's go for this bunch of fixes
mergedToggle commit list