Skip to content

Conversation

@northernSage
Copy link
Member

@northernSage northernSage commented Aug 19, 2021

Addressing #67

  • group semantically related lists used for resource cleanup
  • create a new class XProcessResources to hold all process resources
  • update XProcess.ensure to use XProcessResources
  • add __del__ to XProcessResources so resources are automatically released on end of test execution
  • only explicitly clean up through XProcess._force_resource_cleanup upon interruptions
  • turn xprocess into a package and use find_packages in order to avoid issues such as _internal.py isn't shipped with the 0.18.0 version installed by PyPi #77
  • add changelog

xprocess.py Outdated
# been isued for that particular XProcessInfo
# Object (subprocess requirement)
for (info, _), proc in zip(self._info_objects, self._popen_instances):
for (info, _), proc in zip(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new structure is not a enhancmenet

resource should be a list of a simple classes that contain all resources of a process, just moving attributes into dict keys is not a sensible change, - literally all this does it make the code to access it longer

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall taking some time to figure out all allocated resources when we had the resource leak problem back in #61. It would have been a lot quicker if all resources (Popen instances, XProcessInfo instances and file handles) were all neatly and explicitly grouped in a resources structure such as a dict. So in my opinion this is still an improvement.

That being said, I agree that keeping the resources in a separate class seems like a better way of going about this. If I understand correctly, you're proposing having something like XProcessResources where all resources of a given XProcess instance would be kept (?). If this is the case, we could even further separate concerns by moving the cleanup logic from XProcess._clean_up_resources to XProcessResources.free. @RonnyPfannschmidt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Structurally I would recommend a context manager or a close method to match other common apis

@northernSage
Copy link
Member Author

northernSage commented Sep 4, 2021

We now have a XProcessResources class where all resources used by a running process will be kept. Each XProcessResources knows how to clean itself up by using its release method and will do so upon exit under normal circumstances through its destructor __del__. Upon any interruptions during the test run, the clean-up process will be forced by calling XProcess._force_clean_up in our InterruptionHandler. When you find the time, some feedback on this would be great @RonnyPfannschmidt

@RonnyPfannschmidt
Copy link
Member

Unfortunately I may not be able to get back to to this this weekend

@northernSage northernSage marked this pull request as ready for review October 19, 2021 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants