Better separation between BaseChildWatcher and its subclasses

Better separation between BaseChildWatcher and its subclasses

BaseChildWatcher now cares only about registering the SIGCHLD handler. The _callbacks list is moved into the subclasses.

[GvR, 2013-11-13 20:00:02]

https://codereview.appspot.com/26220043/diff/1/asyncio/unix_events.py
File asyncio/unix_events.py (right):

https://codereview.appspot.com/26220043/diff/1/asyncio/unix_events.py#newcode443
asyncio/unix_events.py:443: def attach_loop(self, loop):
I wonder if these two pieces of functionality (set and clear) shouldn't be
separated.  And is there ever a need to re-attach the same watcher to a
different loop?

https://codereview.appspot.com/26220043/diff/1/asyncio/unix_events.py#newcode536
asyncio/unix_events.py:536: self._callbacks.clear()
Shouldn't this call super().close() last?

https://codereview.appspot.com/26220043/diff/1/asyncio/unix_events.py#newcode610
asyncio/unix_events.py:610: super().close()
Again, IMO super().close() should be called last.

https://codereview.appspot.com/26220043/diff/1/tests/test_events.py
File tests/test_events.py (right):

https://codereview.appspot.com/26220043/diff/1/tests/test_events.py#newcode1316
tests/test_events.py:1316: watcher.attach_loop(self.loop)
Shouldn't you call attach_loop() before calling set_child_watcher()?

https://codereview.appspot.com/26220043/diff/1/tests/test_unix_events.py
File tests/test_unix_events.py (right):

https://codereview.appspot.com/26220043/diff/1/tests/test_unix_events.py#newc...
tests/test_unix_events.py:709: instance = None
You still have this global instance referring to the current test.  That smells
awful. :-)

You're also not doing anything about the stacks of mock.patch decorators on the
various tests.

https://codereview.appspot.com/26220043/diff/1/tests/test_unix_events.py#newc...
tests/test_unix_events.py:718: assert ChildWatcherTestsMixin.instance is None
This assert will still break all tests that follow a test where the
create_watcher() or attach_loop() calls below fail.

[ayba, 2013-11-13 20:43:40]

On 2013/11/13 20:00:02, GvR wrote:
>
https://codereview.appspot.com/26220043/diff/1/asyncio/unix_events.py#newcode443
> asyncio/unix_events.py:443: def attach_loop(self, loop):
> I wonder if these two pieces of functionality (set and clear) shouldn't be
> separated.  And is there ever a need to re-attach the same watcher to a
> different loop?

I don't know. I did this way to keep consistent with other functions that
install a handler (like add_reader, add_child_handler...)

>
https://codereview.appspot.com/26220043/diff/1/asyncio/unix_events.py#newcode536
> asyncio/unix_events.py:536: self._callbacks.clear()
> Shouldn't this call super().close() last?
> 
>
https://codereview.appspot.com/26220043/diff/1/asyncio/unix_events.py#newcode610
> asyncio/unix_events.py:610: super().close()
> Again, IMO super().close() should be called last.

I think it does not make much change (I swapped them anyway).

>
https://codereview.appspot.com/26220043/diff/1/tests/test_events.py#newcode1316
> tests/test_events.py:1316: watcher.attach_loop(self.loop)
> Shouldn't you call attach_loop() before calling set_child_watcher()?

right
 
 
>
https://codereview.appspot.com/26220043/diff/1/tests/test_unix_events.py#newc...
> tests/test_unix_events.py:709: instance = None
> You still have this global instance referring to the current test.  That
smells
> awful. :-)

Well... "cleaner patch" does not mean "perfectly clean" ;)

Yes it smells, as you say. I have to think about it to come up with a better
solution. The fact that the tests are written as blackbox tests impose to keep
some state during the test. Obviously the global state is a concern. How about
the complexity in the mock up functions (needed to be black-box), is it a
concern too ?

> You're also not doing anything about the stacks of mock.patch decorators on
the
> various tests.
> 
>
https://codereview.appspot.com/26220043/diff/1/tests/test_unix_events.py#newc...
> tests/test_unix_events.py:718: assert ChildWatcherTestsMixin.instance is None
> This assert will still break all tests that follow a test where the
> create_watcher() or attach_loop() calls below fail.

Actually I got your reply after sending the patch.

I added the cleanup but I am not comfortable to remove the assert right now
because, as you say, it smells and this assert is the only safeguard to ensure
that the global state is not messed up.

[GvR, 2013-11-13 23:44:55]

We have reached a local optimum of maximum improvement with minimal changes.  I
will commit this to tulip and to CPython.

But I would love it if you could think some more about the patch decorators and
global state.

I wonder if you could just write your own decorator that calls the stack of
patch() calls in context manager mode instead of decorator mode?  It would have
to be something like this:

def my_decorator(func):
  def wrapper(self):
    def waitpid(...): ...
    def WIFEXITED(...): ...
    with unittest.mock.patch('os.waitpid', waitpid), ....:
      func()
    return wrapper

and then in the tests class

  @my_decorator
  def test_something(self):
    ...

[ayba, 2013-11-14 06:37:20]

On 2013/11/13 23:44:55, GvR wrote:
> We have reached a local optimum of maximum improvement with minimal changes. 
I
> will commit this to tulip and to CPython.
> 
> But I would love it if you could think some more about the patch decorators
and
> global state.
> 
> I wonder if you could just write your own decorator that calls the stack of
> patch() calls in context manager mode instead of decorator mode?  It would
have
> to be something like this:
> 
> def my_decorator(func):
>   def wrapper(self):
>     def waitpid(...): ...
>     def WIFEXITED(...): ...
>     with unittest.mock.patch('os.waitpid', waitpid), ....:
>       func()
>     return wrapper
> 
> and then in the tests class
> 
>   @my_decorator
>   def test_something(self):
>     ...

yes, this what I want to do. This is actually not such a huge refactoring.

[ayba, 2013-11-14 07:22:05]

The new patch is here:
https://codereview.appspot.com/26480043