Andrew Wu | 6 Apr 2012 14:29
Picon

MainLoop's pipe is not removed after pipe data ends.

Test code:
===
import urwid
import os

txt = urwid.Text(u"Hello World")
fill = urwid.Filler(txt, 'top')

def show_or_exit(input):
    if input in ('q', 'Q'):
        raise urwid.ExitMainLoop()

loop = urwid.MainLoop(fill, unhandled_input=show_or_exit)

def callback(data):
    if data == "":
        # data end, return False, watch will be removed.
        return False
    return True

fd = loop.watch_pipe(callback)
# write some data into pipe.
f = os.fdopen(fd, "w", 1)
f.write("hello")
f.close()

print "before loop.run():"
print "loop.event_loop._watch_files:"
print loop.event_loop._watch_files
print "loop._watch_pipes:"
print loop._watch_pipes

# press Q to break.
loop.run()

print "after loop.run():"
print "loop.event_loop._watch_files:"
print loop.event_loop._watch_files
print "loop._watch_pipes:"
print loop._watch_pipes
===
Run the test code and press Q to quit, the output is:

before loop.run():
loop.event_loop._watch_files:
{5: <function cb at 0xb7c36144>}
loop._watch_pipes:
{6: (5, 5)}

after loop.run():
loop.event_loop._watch_files:
{}
loop._watch_pipes:
{6: (5, 5)}

You can see that pipe in _watch_files list is removed correctly, but pipe in _watch_pipes list is not removed. I think it is not right.

But if I do this patch:
===
--- urwid_bak/main_loop.py    2012-04-03 19:20:16.000000000 +0800
+++ urwid/main_loop.py    2012-04-06 20:18:20.000000000 +0800
<at> <at> -186,8 +186,7 <at> <at>
             data = os.read(pipe_rd, PIPE_BUFFER_READ_SIZE)
             rval = callback(data)
             if rval is False:
-                self.event_loop.remove_watch_file(watch_handle)
-                os.close(pipe_rd)
+                self.remove_watch_pipe(pipe_wr)
 
         watch_handle = self.event_loop.watch_file(pipe_rd, cb)
         self._watch_pipes[pipe_wr] = (watch_handle, pipe_rd)
===

The output becomes:

before loop.run():
loop.event_loop._watch_files:
{5: <function cb at 0xb7b7b9cc>}
loop._watch_pipes:
{6: (5, 5)}

after loop.run():
loop.event_loop._watch_files:
{}
loop._watch_pipes:
{}

Both _watch_pipes list and _watch_files list are now correct.

_______________________________________________
Urwid mailing list
Urwid <at> lists.excess.org
http://lists.excess.org/mailman/listinfo/urwid
Ian Ward | 10 Apr 2012 21:03
Favicon
Gravatar

Re: MainLoop's pipe is not removed after pipe data ends.

On Fri, Apr 6, 2012 at 8:29 AM, Andrew Wu
> --- urwid_bak/main_loop.py    2012-04-03 19:20:16.000000000 +0800
> +++ urwid/main_loop.py    2012-04-06 20:18:20.000000000 +0800
>  <at>  <at>  -186,8 +186,7  <at>  <at> 
>              data = os.read(pipe_rd, PIPE_BUFFER_READ_SIZE)
>              rval = callback(data)
>              if rval is False:
> -                self.event_loop.remove_watch_file(watch_handle)
> -                os.close(pipe_rd)
> +                self.remove_watch_pipe(pipe_wr)
>
>          watch_handle = self.event_loop.watch_file(pipe_rd, cb)
>          self._watch_pipes[pipe_wr] = (watch_handle, pipe_rd)

That does seem to make more sense.

Now that I'm looking at it though I don't like the interface.  As in
your test code you need to add a comment on "return False" to
understand what is actually going to happen.

Using something like this might be an improvement:

  return "close pipe"

There is also the problem of leaking fds.  The write fd is not closed
by remove_watch_pipe (as documented).  Maybe an optional parameter to
close both ends would help?

Now it seems like there shouldn't be a special way to close a pipe
with a handler's return value (getting too complicated)

Thoughts?

Ian
Andrew Wu | 12 Apr 2012 15:19
Picon

Re: MainLoop's pipe is not removed after pipe data ends.

Ian Ward <ian <at> excess.org> 於 2012年4月11日上午3:03 寫道:

Now it seems like there shouldn't be a special way to close a pipe
with a handler's return value (getting too complicated)

Thoughts?

Yes, this is my idea:
==
--- urwid_bak/main_loop.py    2012-04-03 19:20:16.000000000 +0800
+++ urwid/main_loop.py    2012-04-12 20:44:11.000000000 +0800
<at> <at> -184,10 +184,9 <at> <at>
 
         def cb():
             data = os.read(pipe_rd, PIPE_BUFFER_READ_SIZE)
-            rval = callback(data)
-            if rval is False:
-                self.event_loop.remove_watch_file(watch_handle)
-                os.close(pipe_rd)
+            callback(data)
+            if data == "":
+                self.remove_watch_pipe(pipe_wr)
 
         watch_handle = self.event_loop.watch_file(pipe_rd, cb)
         self._watch_pipes[pipe_wr] = (watch_handle, pipe_rd)
==

The client code gets writer_fd from watch_pipe. The client code is always responsible for closing writer_fd.

If MainLoop object detected the pipe reached EOF(an empty string is read, which means writer_fd is closed, and no more data left in the pipe), it calls remove_watch_pipe to closes reader_fd and does the cleanup.

The client code can also call remove_watch_pipe by itself, but in this case, the client code still need to close writer_fd.

The callback function doesn't need to return any value. I think this interface may be more understandable?

Or, the MainLoop may use Queue to communicate with other threads, by adding methods like watch_queue and remove_watch_queue. In this case, we don't have to worry about leaking fds, and data in Queues is easier to be processed than in pipes.

_______________________________________________
Urwid mailing list
Urwid <at> lists.excess.org
http://lists.excess.org/mailman/listinfo/urwid

Gmane