Re: [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers

[Stefan Hajnoczi]

On Fri, Aug 25, 2017 at 03:32:29PM +0800, Fam Zheng wrote:
> On Thu, 08/24 19:04, Stefan Hajnoczi wrote:
> > On Thu, Aug 24, 2017 at 04:38:43PM +0800, Fam Zheng wrote:
> > > On Thu, 08/24 08:21, Stefan Hajnoczi wrote:
> > > > Tests should declare resources upfront in a with statement.  Resources 
> > > > are
> > > > automatically cleaned up whether the test passes or fails:
> > > > 
> > > >   with FilePath('test.img') as img_path,
> > > >        VM() as vm:
> > > >       ...test...
> > > >   # img_path is deleted and vm is shut down automatically
> > > 
> > > Looks good but still requires test writers to learn and remember to use 
> > > FilePath
> > > and with.
> > 
> > You cannot forget to use FilePath() unless you love typing at
> > os.path.join(iotests.test_dir, 'test.img').  It's much better than open
> > coding filename generation!
> > 
> > > These are still boilerplates.  Here goes my personal oppinion, so may
> > > not be plausible:
> > > 
> > > - For VM() maybe add an atexit in the launch() method also makes sure the 
> > > VM is
> > >   eventually terminated.
> > > 
> > >   This means vm.shutdown() is still needed in tearDown() if there are 
> > > multiple
> > >   test methods and each of them expects a clean state, but that is 
> > > probably
> > >   still less typing (and also indenting) than the with approach, and also 
> > > easy
> > >   to remember (otherwise a test will fail).
> > 
> > I looked into atexit before going this route.  atexit does not have an
> > unregister() API in Python 2.  This makes it ugly to use because some
> > tests do not want the resource to remain for the duration of the
> > process.
> > 
> > A related point is that the Python objects used by atexit handlers live
> > until the end of the process.  They cannot be garbage collected because
> > the atexit handler still has a reference to them.
> 
> I think this shortcoming can be solved with a clean up list ("all problems in
> computer science can be solved by another level of indirection"):
> 
> _clean_up_list = set()
> def _clean_up_handler():
>     for i in _clean_up_list:
>         try:
>             i()
>         except:
>             pass
> 
> atexit.register(_clean_up_handler)
> 
> class VM(...):
> 
>     def launch():
>         ...
>         _clean_up_list.add(self.launch)
> 
>     def shutdown():
>         _clean_up_list.remove(self.launch)
>         ...

atexit is still less powerful than context managers because its scope is
fixed.  Handler functions are only called when the process terminates.
Many test cases do not want resources (especially the VMs) around
forever because they run several iterations or sub-tests.

The with statement can be used both for process-lifetime and for more
fine-grained scoping.  That's why I chose it.

If you stick to atexit then sub-tests or iterations require manual
vm.shutdown() - something that is not necessary using the with
statement.

Stefan