wxExecute synchronous capture output gives "Child process still alive but pipe closed so generating a close notification"

[Rob Bresalier]

Hi:

I've run into the same problem mentioned in the below thread, and think I have found a small bug in wxWidgets:

https://groups.google.com/forum/?fromgroups#!search/still$20alive$20but$20pipe$20closed$20so$20generating$20a$20close$20notification/wx-users/5sCKAHpeaR8/z3hBp0v9jhkJ

It also seems to be addressed by Trac 10258:  http://trac.wxwidgets.org/ticket/10258

I can reproduce this bug by running the exec sample in ubuntu and telling it to 'Capture Command Output', and I execute the '/usr/bin/lsof' command.

I have a solution that does not involve removing the WNOHANG flag to waitpid (as this type of hanging was rejected by Vadim in the previous thread), and I think it is due to a small bug in wxWidgets code.

When waiting for the thread to finish, we have this code inside of DoWaitForChild():

// loop while we're getting EINTR

    for ( ;; )

    {

        rc = waitpid(pid, &status, flags);

        if ( rc != -1 || errno != EINTR )

            break;

    }

The problem is that since the 'flags' parameter is set to WNOHANG, then if the child process is not finished, then waitpid() will return with value 0.  Therefore, we should stay in this loop until the process is finished.  By not providing WNOHANG, we could make sure we don't consume CPU while we are waiting.

I think this code makes a false assumption about what returning '0' means, as is written in a comment a few lines down:

if ( rc == 0 )

    {

        // This can only happen if the child application closes our dummy pipe

        // that is used to monitor its lifetime; in that case, our best bet is

        // to pretend the process did terminate, because otherwise wxExecute()

        // would hang indefinitely (OnReadWaiting() won't be called again, the

        // descriptor is closed now).

        wxLogDebug("Child process (PID %d) still alive but pipe closed so "

                   "generating a close notification", pid);

    }

But this comment is not true, rc==0 does not mean the child closed the dummy pipe, it just means the process is still running.  As is written in the documentation for 'waitpid' from 'http://linux.die.net/man/2/waitpid'

> if WNOHANG was specified and one or more child(ren) specified by pid exist, but have not yet changed state, then 0 is returned

Therefore the loop could be changed to this:

// loop while we're getting EINTR and process is not complete.

    for ( ;; )

    {

        rc = waitpid(pid, &status, flags);

        // Stay in the loop if we get return value 0 or if

        // we are getting EINTR

        if ( (rc==0)||((rc == -1)&&(errno == EINTR )) )

            continue;

        break;

    }

I must admit however that I think using WNOHANG could be better because at least we won't be using CPU while we are waiting. Better to have a zombie wxProcess then an infinite loop consuming CPU.

A compromise solution could be to add a parameter to the API to specify a timeout for how long to wait, so we don't wait forever. 

Another enhancement could be to provide another means (from main thread if this is async execute) to test in this loop to terminate it.

Rob

[Rob Bresalier]

Actually, I think I may have made a mistake in this description, as I am calling wxExecute synchronously, it seems that WNOHANG is not provided to waitpid() in the synchronous case.  I need to rebuild wx with debugging turned on and step through with a debugger to make sure.

[Rob Bresalier]

OK, the debugger shows that WNOHANG is getting used, so my original email stands.  Seems like we should keep waiting when waitpid() returns 0.

[Rob Bresalier]

Another finding - if I set a breakpoint at waitpid() and delay so that I know the external process 'lsof' is done, then waitpid() still returns 0.

My speculation is that the external process may be closing the input pipe (as it is not needed), but this should not be treated as a problem.  I'm not sure I understand exactly what is happening.  More investigation needed.  If anyone has any insight, please let me know!

[Rob Bresalier]

I did some more investigation of this issue and I found that I have to agree with the previous posters who said to take away WNOHANG in wxHandleProcessTermination().  This seems to be the only solution that I could find, and it works beautifully.

This was suggested in the Trac 10258:

http://trac.wxwidgets.org/ticket/10258

The fact that we are inside of wxHandleProcessTermination() means the process is terminated, so I don't see a risk to block on waitpid().

I tried putting a wait loop as I wrote in my original email, but it waited forever.  It seems there is no choice but to call waitpid() without WNOHANG.  I pasted my wait loop below.

Also, I don't see why this comment in the code can be true:

        // This can only happen if the child application closes our dummy pipe

        // that is used to monitor its lifetime; in that case, our best bet is

        // to pretend the process did terminate, because otherwise wxExecute()

        // would hang indefinitely (OnReadWaiting() won't be called again, the

        // descriptor is closed now).

I don't see anywhere in the documentation of waitpid() that says that it can return 0 if the child application closes the dummy pipe.  So where does this statement come from?

for ( ;; )

    {

        rc = waitpid(pid, &status, flags);

        // Stay in the loop if we get return value 0 or if

        // we are getting EINTR

        if ( (rc==0)||((rc == -1)&&(errno == EINTR )) )

            continue;

        break;

    }

Rob Bresalier

[Vadim Zeitlin]

On Mon, 31 Dec 2012 13:38:45 -0500 Rob Bresalier wrote:

RB> I did some more investigation of this issue and I found that I have to
RB> agree with the previous posters who said to take away WNOHANG in
RB> wxHandleProcessTermination(). This seems to be the only solution that I
RB> could find, and it works beautifully.

Sorry, it doesn't. It works beautifully in your case when you have a
program that exits quickly. If you use it with a program that closes all
its FDs and then keeps running for a long time, this solution results in
hanging the wxWidgets process. This is much worse than having zombies in
the process table, so even if the current situation is absolutely not
perfect, let's not make it even worse.

RB> This was suggested in the Trac 10258:
RB>
RB> http://trac.wxwidgets.org/ticket/10258

Yes, but this unfortunately doesn't make it right. I'll leave a note
there.

RB> The fact that we are inside of wxHandleProcessTermination() means the
RB> process is terminated, so I don't see a risk to block on waitpid().

No, the fact that we are inside wxHandleProcessTermination() means that
our pipe has been closed, nothing else. It was just a fundamental mistake
to assume that the pipe was closed only on process termination as the
original author of this code did because this is not true and many
processes close all their open FDs, including our pipe, long before
exiting.

RB> I don't see anywhere in the documentation of waitpid() that says that it
RB> can return 0 if the child application closes the dummy pipe. So where does
RB> this statement come from?

From the fact that we only run this code if the pipe was closed.

Hope I managed to explain the problem this time. Unfortunately I'm pretty
sure there is simply no good solution not involving rewriting this code
completely.

Regards,
VZ

--
TT-Solutions: wxWidgets consultancy and technical support
http://www.tt-solutions.com/

[Rob Bresalier]

What about this:  

If we find that waitpid() is returning a 0 with WNOHANG, then we start a new thread that will call a blocking waitpid() without WNOHANG, would that work?  Then we can get rid of the zombie process, and also not block the main thread.

I don't understand why the OS is returning 0 to waitpid() with WNOHANG when I know that the process has terminated - as I said that I experimented with a loop that polled the output of waitpid() and found it was staying at 0, and never returning the pid, even after the process terminated.

[Vadim Zeitlin]

On Wed, 2 Jan 2013 11:06:43 -0500 Rob Bresalier wrote:

RB> If we find that waitpid() is returning a 0 with WNOHANG, then we start a
RB> new thread that will call a blocking waitpid() without WNOHANG, would that
RB> work?

It would, but we'd need to take care of handling this thread life time,
i.e. making sure we terminate it on program exit, and if we do this anyhow
then it would make sense to just have a thread doing this from the very
beginning and call waitpid(-1, ...) on it and not use the dummy pipe at
all. This is what I meant by "rewriting this code".

OTOH I am not sure about the interactions of threads vs waitpid() on all
the Unix systems. So I still think that just catching SIGCHLD would be an
even simpler and more portable solution.

RB> I don't understand why the OS is returning 0 to waitpid() with WNOHANG when
RB> I know that the process has terminated - as I said that I experimented with
RB> a loop that polled the output of waitpid() and found it was staying at 0,
RB> and never returning the pid, even after the process terminated.

I don't know this neither, but perhaps it only happens while the process
is being stopped by the debugger?

[Rob Bresalier]

VZ>  I don't know this neither, but perhaps it only happens while the process
VZ> is being stopped by the debugger?

I also ran it outside of the debugger and it would not get out of the loop.  I don't know why.

[Rob Bresalier]

I've been experimenting with using SIGCHLD to detect the process termination.  I have studied the bowels of the wx code and I understand how it works.  I have found the wx routines wxAppConsole::SetSignalHandler() and wxAppConsole::CheckSignal() that are to be used for handling signals.

I'm finding that my signal handler (to be called by wxAppConsole::CheckSignal()), is never getting called.  I think it is because wxAppConsole::CheckSignal() is only called inside of wxConsoleEventLoop::OnNextIteration().  However, in a GUI program the wxConsoleEventLoop::OnNextIteration() does not execute, therefore CheckSignal() has no opportunity to execute and we can never handle it.

The signal interrupt function that wx provides, which is wxAppConsole::HandleSignal() calls app->WakeUpIdle(), which I think is there so that we can call CheckSignal() in idle processing, but the problem is that there is no call to CheckSignal() in the idle processing.

Therefore, I think that a call to wxAppConsole::CheckSignal() should be placed inside of wxAppBase::ProcessIdle() (it should be wrapped with an #ifdef for UNIX implementation however).

Vadim:  Would there be any issue adding a call to wxAppConsole::CheckSignal() inside of wxAppBase::ProcessIdle()?

Once I get this working, I would like to provide a patch.  To be safe, I will make this new method of detecting process termination only enabled with a new flag to wxExecute().  Without this new flag present, we will default to the old method of using the dummy pipe.

My implementation keeps a list of PIDS of child processes that have been spawned.  Then I have a function that I named wxCheckChildProcessTermination(), which goes through this list and calls waitpid(pid,&status,WNOHANG) on each of the open PIDS.  If this waitpid function returns the pid, then we have detected the process termination and we call wxHandleProcessTermination().

Of course I would call the new wxCheckChildProcessTermination() in my signal handler for SIGCHLD (as a result of wx calling wxAppConsole::CheckSignal().

However, I think a more robust solution would to also add the call to wxCheckChildProcessTermination() inside of wxAppBase::ProcessIdle().  Then we are constantly checking the opened pids for termination.  What do you think of this idea?

Rob

[Vadim Zeitlin]

On Sat, 5 Jan 2013 02:12:44 -0500 Rob Bresalier wrote:

RB> I've been experimenting with using SIGCHLD to detect the process
RB> termination. I have studied the bowels of the wx code and I understand how
RB> it works. I have found the wx routines wxAppConsole::SetSignalHandler()
RB> and wxAppConsole::CheckSignal() that are to be used for handling signals.
RB>
RB> I'm finding that my signal handler (to be called
RB> by wxAppConsole::CheckSignal()), is never getting called. I think it is
RB> because wxAppConsole::CheckSignal() is only called inside
RB> of wxConsoleEventLoop::OnNextIteration().

Yes, you're absolutely correct, thanks for diving into it, I completely
forgot (or perhaps even just didn't know) about this myself.

RB> Vadim: Would there be any issue adding a call
RB> to wxAppConsole::CheckSignal() inside of wxAppBase::ProcessIdle()?

If we do it like this, we'd also need to remove it from Unix version of
wxConsoleEventLoop::OnNextIteration() as otherwise it would be called
twice. However other than that I don't see any problems with doing it like
this.

OTOH it doesn't exactly seem like the most elegant solution neither, idle
time processing and signal handling are quite different. The first can be
postponed arbitrarily late while the latter should usually be done a.s.a.p.
i.e. they have quite different priorities. They also basically have nothing
to do with each other so it's a bit strange to mix them like this.

Unfortunately the only solution I see is quite a bit more difficult than
just adding a call to CheckSignal(): we'd need to use the usual "self-pipe
trick", i.e. create a pipe, add the read end of the pipe to the event loop
using AddSourceForFD() and replace the wxWakeUpIdle() call in HandleSignal()
with writing something to the write end of this pipe, thus waking the event
loop and resulting in our handler being called. This is, of course, the
same technique that is used by wxConsoleEventLoop for its m_wakeupPipe, so
the same PipeIOHandler class could/should be reused for this, except that
we need to avoid using the lock somehow in this case because it would be
unnecessary and I don't even know if locking a mutex is safe inside a
signal handler.

The problem is that while this solution seems to be more elegant to me I
can't actually point out to any concrete advantages of it compared to just
checking for the signals during the idle time so I'm not totally sure if
it's worth spending time and effort on doing it like this.


RB> Once I get this working, I would like to provide a patch. To be safe, I
RB> will make this new method of detecting process termination only enabled
RB> with a new flag to wxExecute(). Without this new flag present, we will
RB> default to the old method of using the dummy pipe.

I'd love to have a patch fixing this but I really don't think we should
use 2 different methods. Either the patch will work, in which case it
should be always used, or it won't, in which case it shouldn't be applied
at all. I'd strongly prefer to remove the existing pipe-based end detection
stuff and use just SIGCHLD instead.

RB> My implementation keeps a list of PIDS of child processes that have been
RB> spawned. Then I have a function that I named
RB> wxCheckChildProcessTermination(), which goes through this list and calls
RB> waitpid(pid,&status,WNOHANG) on each of the open PIDS. If this waitpid
RB> function returns the pid, then we have detected the process termination and
RB> we call wxHandleProcessTermination().

Why not call waitpid(-1, &status, WNOHANG) as I proposed? This should be
much more efficient in case of many child processes as you'd only pay the
overhead of a system call once. If you do it like this, you probably should
be calling waitpid(-1) in a loop while it returns non-null values as more
than one child could have terminated. And you'd also want to use a hash of
child processes instead of a list to be able to quickly find them by their
PID.

RB> However, I think a more robust solution would to also add the call
RB> to wxCheckChildProcessTermination() inside of wxAppBase::ProcessIdle().
RB> Then we are constantly checking the opened pids for termination. What do
RB> you think of this idea?

This really shouldn't be necessary and could result in noticeable slow
down in the program if it launched many child processes.

[Rob Bresalier]

Thanks Vadim for your answers.

Please see my responses below.  

Also, I have an additional thought about how to handle the wait loop for sync execution in console applications and I want to run it by you.  Currently, the console wait loop for synchronous execution is this:

while ( !endHandler.Terminated() )

{

    disp.Dispatch();

}

The disp.Dispatch() makes a blocking call to select() with no timeout.  I think it would impossible to get my signal handler in there, as we have no call to ProcessIdle() here or an event handler.

Therefore I think that I should call Dispatch() with a timeout, and then check for terminated processes after the timeout.  I was thinking 10ms, do you think that is a good value, or I should choose another one?  Then, the loop would become this:

int rc=0;

do                                                                                

{                                                                                 

    if (rc==-1)                                                                   

    {  // This can happen if all the file descriptors are closed, but the program 

       // has not terminated yet.  To keep us from burning CPU in this case, we   

       // will sleep for 10ms                                                     

       wxMilliSleep(10);                                                          

    }                                                                             

                                                                                  

    rc=disp.Dispatch(10);  // select() with timeout of 10ms.                      

                        // timeout is necessary so we can periodically check      

                        // to see if our child process has terminated.            

                                                                                  

    // If this function finds that the pid that we care about has                 

    // finished, then execData.endProcData.pid will be set to non-zero,           

    // which will cause us to exit this loop.                                     

    wxCheckChildProcessTermination(SIGCHLD);                                      

} while(execData.endProcData.pid != 0);                                           

What do you think about this?

Also, about the sync wait loop in GUI applications.  If we put the signal handler in the idle function, are we sure it would get called in wxYield()?  As a reminder, the sync wait loop in GUI applications is this:

while ( endProcData->pid != 0 )

    {

        // don't consume 100% of the CPU while we're sitting in this

        // loop

        if ( !CheckForRedirectedIO(execData) )

            wxMilliSleep(1);

        // give the toolkit a chance to call wxHandleProcessTermination() here

        // and also repaint the GUI and handle other accumulated events

        wxYield();

    }

What I have done in my local implementation, which seems to work, is to add a call to wxCheckChildProcessTermination(SIGCHLD) after the call to wxYield().  This seems to work fine, but of course, it only handles the synchronous case, and not the asynchronous case:

while ( endProcData->pid != 0 )

    {

        // don't consume 100% of the CPU while we're sitting in this

        // loop

        if ( !CheckForRedirectedIO(execData) )

            wxMilliSleep(1);

        // give the toolkit a chance to call wxHandleProcessTermination() here

        // and also repaint the GUI and handle other accumulated events

        wxYield();

        // wxYield() does not seem to have a call to CheckSignal() in it,

        // therefore every iteration we will check if the process has ended.

        // This only applies to the next wxEXEC_USE_SIGCHLD

        wxCheckChildProcessTermination(SIGCHLD);

    }

RB> Once I get this working, I would like to provide a patch.  To be safe, I
RB> will make this new method of detecting process termination only enabled
RB> with a new flag to wxExecute().  Without this new flag present, we will
RB> default to the old method of using the dummy pipe.

 I'd love to have a patch fixing this but I really don't think we should
use 2 different methods. Either the patch will work, in which case it
should be always used, or it won't, in which case it shouldn't be applied
at all. I'd strongly prefer to remove the existing pipe-based end detection
stuff and use just SIGCHLD instead.

If we use this fix and get rid of the old implementation, then I'll need to make sure that I did not break anything.  I currently have the ability to test on Mac and Ubuntu.  If it passes tests on those 2 platforms, would you be confident enough in it?  Also, what about test cases?  I have not explored the existing unit tests yet for wxExecute(), is there good coverage in there?  I think I should test at least 3 cases:  async execution in GUI app, sync execution in GUI app, and sync execution in console app.  I have code available for testing the 2 GUI cases, but I don't have anything available for testing a console app.  Do you know of an existing console app that I can use to test it?  Otherwise, I'll need to roll my own.

 

 Why not call waitpid(-1, &status, WNOHANG) as I proposed? 

I thought about doing it that way and perhaps I should. I'm just scared that while a few child processes are terminated that I might get a return value of 0 because of WNOHANG and then if I made a subsequent call it would return the PID.  But since I would be looping as you said, if I got a return value of 0 that would cause an exit from the loop (and perhaps an error return of -1 should also cause the loop to stop).  Because the loop would be exited, there would be no subsequent call to find another PID that terminated.  This fear is probably unfounded, so with a little reassurance that this would not happen I can do it that way.  I already implemented my list as a hash, so it would be a small change.

 

RB> However, I think a more robust solution would to also add the call
RB> to wxCheckChildProcessTermination() inside of wxAppBase::ProcessIdle().
RB>  Then we are constantly checking the opened pids for termination.  What do
RB> you think of this idea?

 This really shouldn't be necessary and could result in noticeable slow
down in the program if it launched many child processes.

Again, I need a little reassurance that this should not be necessary.  For example, I see a possible race condition.  I need to store the PID, but I don't know the PID until after the call to fork().  So what happens if we get SIGCHLD after fork() but before I stored the PID?  I think this can be mitigated if I store the PID in the hash immediately after the call to fork() - to be sure that no event loop executes that handles my SIGCHLD handler before I've stored the PID in the hash.  Also, can we always be absolutely sure we'll never miss a SIGCHLD?  Perhaps I should use a timer to periodically check it if it is too much overhead to put it in idle processing?  If you agree with using a timer, what would be a good timeout value?  And which class would be a good spot to implement this timer (such as in one of the base classes of wxApp)?

 

[Rob Bresalier]

I just found a reason why we may not want to use waitpid(-1, &status, WNOHANG).

While I was debugging my code I triggered an assert.  During the assert processing, the ~wxStdioPipe() got called.  Inside this function is pclose(m_fp).  This pclose() triggered a waitpid (I see it in the stack trace), and then I received a SIGCHLD during the waitpid (because the stack trace shows the waitpid() was interrupted by the signal handler, and the debugger shows it is signal 17 - SIGCHLD).

Therefore, I think it is only safe to call waitpid() on the pids that we know about.  In this particular case, since the signal handler occurs during the call to waitpid, then I would probably not see it again when I eventually called waitpid(-1,&status,WNOHANG), but seeing this happen made me nervous - that is we probably don't want to do a waitpid on pids that the OS may care about.

[Vadim Zeitlin]

On Sat, 5 Jan 2013 15:34:13 -0500 Rob Bresalier wrote:

RB> Currently, the console wait loop for synchronous execution is this:
RB>
RB> while ( !endHandler.Terminated() )
RB> {
RB> disp.Dispatch();
RB> }
RB>
RB> The disp.Dispatch() makes a blocking call to select() with no timeout. I
RB> think it would impossible to get my signal handler in there,

If we use the self-pipe trick it would be possible, and even easy, though:
we'd just need to register the pipe that is written to by the signal
handler with the dispatcher. And this is really the only real solution,
anything else would be just a dirty hack :-(

RB> Therefore I think that I should call Dispatch() with a timeout, and then
RB> check for terminated processes after the timeout. I was thinking 10ms, do
RB> you think that is a good value, or I should choose another one?

There is really no good value, 10ms seems a bit on the short side but is
probably fine for the relatively short waits. Avoiding polling completely
would still be much better though...

RB> Also, about the sync wait loop in GUI applications. If we put the signal
RB> handler in the idle function, are we sure it would get called in wxYield()?
RB> As a reminder, the sync wait loop in GUI applications is this:

This loop is quite horrible too, of course. I have no idea why don't we
use a local event loop with the same self-pipe approach to handle IO. More
I look at this code, more I realize just how completely broken it is :-(

RB> If we use this fix and get rid of the old implementation, then I'll need to
RB> make sure that I did not break anything. I currently have the ability to
RB> test on Mac and Ubuntu. If it passes tests on those 2 platforms, would you
RB> be confident enough in it?

Yes, I think so.

RB> Also, what about test cases? I have not explored the existing unit
RB> tests yet for wxExecute(), is there good coverage in there?

There are a few tests in tests/exec/exec.cpp but they're clearly not
exhaustive. In particular, they don't test with any process that closes all
its descriptors before terminating and so don't catch the current problem.

RB> Do you know of an existing console app that I can use to test it?

The test program is a console one, but actually I think we should probably
run these tests in GUI test program too, i.e. duplicate exec.cpp in the
test_gui section of tests/test.bkl file, as it's already done for a few
other tests in order to check that it works correctly in both cases.

RB> > Why not call waitpid(-1, &status, WNOHANG) as I proposed?
[...]

Let's leave this aside for now, replacing a loop waiting for all children
with a single call to waitpid(-1) can always be done later.

RB> Again, I need a little reassurance that this should not be necessary. For
RB> example, I see a possible race condition. I need to store the PID, but I
RB> don't know the PID until after the call to fork(). So what happens if we
RB> get SIGCHLD after fork() but before I stored the PID?

This shouldn't be a problem because we don't do anything immediately when
we get the signal but only process it later, when we return to the event
loop. And by this time the PID should be already stored. Or am I missing
something here?

RB> Also, can we always be absolutely sure we'll never miss a SIGCHLD?

I think so. At least I don't know about any circumstances when this could
legitimately happen. OTOH we do need to take care to not lose the signal
notification once we get it because we won't get it again. But, again, I
think things are made much simpler by the fact that we serialize everything
via the event loop, this simplifies the logic significantly compared to the
normal case.

RB> Perhaps I should use a timer to periodically check it if it is too much
RB> overhead to put it in idle processing? If you agree with using a
RB> timer

I'd really prefer to avoid this. Our code should be correct instead of
relying on somebody else to clean up after it...

[Rob Bresalier]

Thanks again Vadim.

One more question:  I would like to call SetSignalHandler() at the start of the program to establish the SIGCHLD signal handler.  Where would be a good place to put this function call at the start of a program?  

Additionally, I see many cases in the wx library where it is possible that wxTheApp may not be defined.  I was able to make a situation like this by using the console sample and commenting out the instantiation of wxInitializer.  Since SetSignalHandler() and a few other signal functions (like CheckSignal(), and also the hash that keeps track of the signals) require a wxTheApp to exist, how to handle this situation for programs that don't have an app object?

On Sun, Jan 6, 2013 at 8:12 AM, Vadim Zeitlin <va...@wxwidgets.org> wrote:

On Sat, 5 Jan 2013 15:34:13 -0500 Rob Bresalier wrote:

RB>  Currently, the console wait loop for synchronous execution is this:
RB>
RB> while ( !endHandler.Terminated() )
RB> {
RB>     disp.Dispatch();
RB> }
RB>
RB> The disp.Dispatch() makes a blocking call to select() with no timeout.  I
RB> think it would impossible to get my signal handler in there,

 If we use the self-pipe trick it would be possible, and even easy, though:
we'd just need to register the pipe that is written to by the signal
handler with the dispatcher. And this is really the only real solution,
anything else would be just a dirty hack :-(

I'm looking into the self-pipe trick.  I'll need to move the definition of PipeIOHandler out of evtloopunix.cpp into a header file so I can reuse it.  Do you have a good suggestion for which header file to use?

I'm also trying to think of a way to use it without the lock.  The lock seems to be there to protect the m_pipeIsEmpty member variable.  I think I could replace it with 2 member variables, such as:  m_pipeReadCount and m_pipeWriteCount.  Only the serialized signal handler function would write to m_pipeWriteCount (actually, it would increment it by 1), and only the OnReadWaiting() would write (increment by 1) to m_pipeReadCount.  

In the serialized signal handler, if m_pipeWriteCount is equal to m_pipeReadCount or equal to m_pipeReadCount+1, then it would put data in the dummy pipe to trigger, after this the m_pipeWriteCount would get incremented.  When the OnReadWaiting() happens, it increments m_pipeReadCount by 1.

By testing if m_pipeWriteCount is equal to both m_pipeReadCount or m_pipeReadCount+1, we can make sure we always handle the condition where we interrupted the incrementing of m_pipeReadCount in OnReadWaiting().  It means we will trigger the pipe a little more than we would have in the old implementation, but at the same time it also avoids flooding the pipe as well.

 

RB> Therefore I think that I should call Dispatch() with a timeout, and then
RB> check for terminated processes after the timeout.  I was thinking 10ms, do
RB> you think that is a good value, or I should choose another one?

 There is really no good value, 10ms seems a bit on the short side but is
probably fine for the relatively short waits. Avoiding polling completely
would still be much better though...

RB> Also, about the sync wait loop in GUI applications.  If we put the signal
RB> handler in the idle function, are we sure it would get called in wxYield()?
RB> As a reminder, the sync wait loop in GUI applications is this:

 This loop is quite horrible too, of course. I have no idea why don't we
use a local event loop with the same self-pipe approach to handle IO. More
I look at this code, more I realize just how completely broken it is :-(

I was thinking the a local event loop would be much better too, but did not want to touch too many things.  I have worked with them before though, so I think I can try it.

Can you just confirm for me, I would use wxConsoleEventLoop for the console event loop in the WaitForChild() in utilsunx.cpp, and I would use wxGUIEventLoop inside of WaitForChild() in apptraits.cpp?

 

RB> Again, I need a little reassurance that this should not be necessary.  For
RB> example, I see a possible race condition.  I need to store the PID, but I
RB> don't know the PID until after the call to fork().  So what happens if we
RB> get SIGCHLD after fork() but before I stored the PID?

 This shouldn't be a problem because we don't do anything immediately when
we get the signal but only process it later, when we return to the event
loop. And by this time the PID should be already stored. Or am I missing
something here?

Yes, I have seen sometimes that when we call some wx functions or instantiate objects, that we may encounter a temporary event loop or execute a GTK call that may trigger handling of something (like processing our dummy pipe handler).  I think it is possible to avoid this from happening by storing the PID in our hash/list immediately after the fork, before we have any chance to call any function or instantiate any object that could make a system call.  As long as we are sure there is no intervening system call to GTK or anything else that can trigger the handling of the dummy pipe inbetween the fork() and storing it in the hash, we should be ok.

[Vadim Zeitlin]

On Sun, 6 Jan 2013 11:09:30 -0500 Rob Bresalier wrote:

RB> One more question: I would like to call SetSignalHandler() at the start of
RB> the program to establish the SIGCHLD signal handler. Where would be a good
RB> place to put this function call at the start of a program?

I actually don't think it needs to be done at the start of the program.
Why not do it only when wxExecute() is called for the first time?

RB> Additionally, I see many cases in the wx library where it is possible that
RB> wxTheApp may not be defined. I was able to make a situation like this by
RB> using the console sample and commenting out the instantiation of
RB> wxInitializer.

This is wrong. The library _must_ be initialized. IOW the absence of
wxTheApp by the time wxExecute() is called should result in an assertion
failure and error return.

RB> I'm looking into the self-pipe trick. I'll need to move the definition
RB> of PipeIOHandler out of evtloopunix.cpp into a header file so I can reuse
RB> it. Do you have a good suggestion for which header file to use?

Probably a new file under include/wx/unix/private. As for the name of the
file... selfpipe.h perhaps?

RB> I'm also trying to think of a way to use it without the lock.

I think the simplest would be to have a thread-unsafe base class and a
derived class which would add a lock around the calls to the base class
methods. E.g.

class MTSafePipeIOHandler : public PipeIOHandler {
public:
virtual void WakeUp() {
wxCriticalSectionLocker lock(m_pipeLock);
PipeIOHandler::WakeUp();
}

virtual void OnReadWaiting() {
wxCriticalSectionLocker lock(m_pipeLock);
PipeIOHandler::OnReadWaiting();
}

private:
wxCriticalSection m_pipeLock;
};

should be all we need AFAICS.


RB> I was thinking the a local event loop would be much better too, but did not
RB> want to touch too many things.

This is certainly the right idea, you shouldn't try to change everything
at once. I am just despairing at how we completely failed to think about a
better way to do all this when this code was initially written. I'm pretty
sure that if we had thought about doing it right, we would have found a way
but it really looks like this code was not designed at all but just hacked
on until it somehow started to work.

RB> Can you just confirm for me, I would use wxConsoleEventLoop for the console
RB> event loop in the WaitForChild() in utilsunx.cpp, and I would
RB> use wxGUIEventLoop inside of WaitForChild() in apptraits.cpp?

I think so, yes.


RB> Yes, I have seen sometimes that when we call some wx functions or
RB> instantiate objects, that we may encounter a temporary event loop or
RB> execute a GTK call that may trigger handling of something (like processing
RB> our dummy pipe handler).

The only function that could result in it would be wxYield() -- which is
why it's so dangerous. But it really shouldn't happen otherwise (of course,
there are some functions/classes that use wxYield internally, e.g.
wxBusyInfo, but there are very few of them and usually it's not a surprise
that they do it because they work a bit like wxYield).

[Rob Bresalier]

To use the self-pipe with signals, I need to make a modification to the AddProcessCallback() function.  This function is used to register the callback that will occur when there is data on the read end of the self pipe.

The modification is necessary because the current implementation will close the self-pipe (as it was used for the dummy pipe for process end determination).

However, for using SIGCHLD, I need to keep this pipe open, as I could have several child processes running, and the signal interrupt handler can only pass data to 1 fd, as all the signal handler knows is that it received SIGCHLD and has no other information.

Now the implementation of AddProcessCallback() is platform specific.  Therefore there are 5 different implementations of it.  I have listed them below:

1) GTK

2) OSX

3) Unix console

4) GTK1

5) Motif

Now I will be able to test the first 3 implementations, GTK, OSX, and unix console, but I have never dealt with gtk1 or motif before.  I think these ports are probably obsolete anyway.  I will modify the code for them, but I won't be able to test or build it.  Is that OK?

Also, please see responses below:

On Sun, Jan 6, 2013 at 12:19 PM, Vadim Zeitlin <va...@wxwidgets.org> wrote:

On Sun, 6 Jan 2013 11:09:30 -0500 Rob Bresalier wrote:

RB> One more question:  I would like to call SetSignalHandler() at the start of
RB> the program to establish the SIGCHLD signal handler.  Where would be a good
RB> place to put this function call at the start of a program?

 I actually don't think it needs to be done at the start of the program.
Why not do it only when wxExecute() is called for the first time?

Good idea.  I will do it that way.

 

RB> Additionally, I see many cases in the wx library where it is possible that
RB> wxTheApp may not be defined.  I was able to make a situation like this by
RB> using the console sample and commenting out the instantiation of
RB> wxInitializer.

 This is wrong. The library _must_ be initialized. IOW the absence of
wxTheApp by the time wxExecute() is called should result in an assertion
failure and error return.

OK, but the existing code in wxExecute() does not assert, but allows this to happen.  Look at these lines inside of wxExecute():

// we want this function to work even if there is no wxApp so ensure

// that we have a valid traits pointer

wxConsoleAppTraits traitsConsole;

wxAppTraits *traits = wxTheApp ? wxTheApp->GetTraits() : NULL;

if ( !traits )

    traits = &traitsConsole;

 

RB> I'm looking into the self-pipe trick.  I'll need to move the definition
RB> of PipeIOHandler out of evtloopunix.cpp into a header file so I can reuse
RB> it.  Do you have a good suggestion for which header file to use?

 Probably a new file under include/wx/unix/private. As for the name of the
file... selfpipe.h perhaps?

I created include/wx/private/pipeiohandler.h

 

RB> I'm also trying to think of a way to use it without the lock.

 I think the simplest would be to have a thread-unsafe base class and a
derived class which would add a lock around the calls to the base class
methods. E.g.

        class MTSafePipeIOHandler : public PipeIOHandler {
        public:
                virtual void WakeUp() {
                        wxCriticalSectionLocker lock(m_pipeLock);
                        PipeIOHandler::WakeUp();
                }

                virtual void OnReadWaiting() {
                        wxCriticalSectionLocker lock(m_pipeLock);
                        PipeIOHandler::OnReadWaiting();
                }

        private:
                wxCriticalSection m_pipeLock;
        };

should be all we need AFAICS.

OK, I think you are right that it is safe to use a lock-less version in a signal handler, and it will still work no matter where in the code the signal handler is interrupting.

 

RB> Yes, I have seen sometimes that when we call some wx functions or
RB> instantiate objects, that we may encounter a temporary event loop or
RB> execute a GTK call that may trigger handling of something (like processing
RB> our dummy pipe handler).

 The only function that could result in it would be wxYield() -- which is
why it's so dangerous. But it really shouldn't happen otherwise (of course,
there are some functions/classes that use wxYield internally, e.g.
wxBusyInfo, but there are very few of them and usually it's not a surprise
that they do it because they work a bit like wxYield).

I've noticed that this instantiation inside of wxGUIAppTraits::WaitForChild() seems to invoke a gtk call and I suspect it could invoke the self-pipe handler:

wxBusyCursor bc;

I had this captured in a stack trace in the debugger, but I've done some development and I'm not in a good state right now to rebuild and find it again.  I will give more information on it later.

[Rob Bresalier]

Some corrections:

1) New header file is: include/wx/unix/private/pipeiohandler.h (I forgot the 'unix' part).

2) When I said I needed to change to AddProcessCallback(), I meant I need to change the callback function that it causes to get called by the OS.  In each platform there is a different callback function that closes the pipe, and I need to get rid of that pipe closure.  I'm also going to make a small change to one of the parameter types of AddProcessCallback(), to pass a pointer to PipeIOHandler *proc_data as below:

AddProcessCallback(PipeIOHandler *proc_data, int fd)

This is useful for the console version of AddProcessCallback, so that I can pass my self-pipe directly to the RegisterFD() inside of it.  For the different GUI versions, this pointer gets passed to the callback so it does not matter what type it is.

[Vadim Zeitlin]

On Mon, 7 Jan 2013 00:39:33 -0500 Rob Bresalier wrote:

RB> Now the implementation of AddProcessCallback() is platform specific.
RB> Therefore there are 5 different implementations of it. I have listed them
RB> below:
RB>
RB> 1) GTK
RB> 2) OSX
RB> 3) Unix console
RB> 4) GTK1
RB> 5) Motif
RB>
RB> Now I will be able to test the first 3 implementations, GTK, OSX, and unix
RB> console, but I have never dealt with gtk1 or motif before. I think these
RB> ports are probably obsolete anyway. I will modify the code for them, but I
RB> won't be able to test or build it. Is that OK?

Yes, it should be. I don't follow all the details here but I think the
code should be relatively straightforward for GTK1 and Motif too.

BTW, about following: could you please upload the patch when it's ready to
review.bakefile.org? This would make it simpler to comment on it. TIA!

RB> > This is wrong. The library must be initialized. IOW the absence of
RB> > wxTheApp by the time wxExecute() is called should result in an assertion
RB> > failure and error return.
RB>
RB> OK, but the existing code in wxExecute() does not assert, but allows this
RB> to happen. Look at these lines inside of wxExecute():
RB>
RB> // we want this function to work even if there is no wxApp so ensure
RB> // that we have a valid traits pointer
RB> wxConsoleAppTraits traitsConsole;
RB> wxAppTraits *traits = wxTheApp ? wxTheApp->GetTraits() : NULL;
RB> if ( !traits )
RB> traits = &traitsConsole;

I think we could safely remove this and just assert instead (although
there is no hurry to do it, of course). IMHO the effort to make sure
everything works before OnInit() or after OnExit() is just not worth it.

RB> > RB> Yes, I have seen sometimes that when we call some wx functions or
RB> > RB> instantiate objects, that we may encounter a temporary event loop or
RB> > RB> execute a GTK call that may trigger handling of something (like
RB> > processing
RB> > RB> our dummy pipe handler).
RB> >
RB> > The only function that could result in it would be wxYield() -- which is
RB> > why it's so dangerous. But it really shouldn't happen otherwise (of course,
RB> > there are some functions/classes that use wxYield internally, e.g.
RB> > wxBusyInfo, but there are very few of them and usually it's not a surprise
RB> > that they do it because they work a bit like wxYield).
RB> >
RB> I've noticed that this instantiation inside of
RB> wxGUIAppTraits::WaitForChild() seems to invoke a gtk call and I suspect it
RB> could invoke the self-pipe handler:
RB>
RB> wxBusyCursor bc;

wxBusyCursor does call gdk_display_flush() but I don't think it's supposed
to handle any wx events internally...

[Rob Bresalier]

 BTW, about following: could you please upload the patch when it's ready to
review.bakefile.org? This would make it simpler to comment on it. TIA!

 Will do.  There are changes in lots of files, definitely should be reviewed and make it easy to comment on.

 wxBusyCursor does call gdk_display_flush() but I don't think it's supposed
to handle any wx events internally...

Its not wx events that I'm so worried about.  GTK may invoke the callback that gets triggered when we write to our self-pipe.  So I just need to make sure to save the PID before any function call that could possibly trigger our callback.  Inside our callback I will go through our list of child processes and attempt waitpid() on them, and then call OnTerminate().  Or, do you prefer I post an event from this function instead, so that we do this processing as a wx event?

[Vadim Zeitlin]

On Mon, 7 Jan 2013 09:37:52 -0500 Rob Bresalier wrote:

RB> > wxBusyCursor does call gdk_display_flush() but I don't think it's supposed
RB> > to handle any wx events internally...
RB> >
RB> Its not wx events that I'm so worried about. GTK may invoke the callback
RB> that gets triggered when we write to our self-pipe. So I just need to make
RB> sure to save the PID before any function call that could possibly trigger
RB> our callback. Inside our callback I will go through our list of child
RB> processes and attempt waitpid() on them, and then call OnTerminate(). Or,
RB> do you prefer I post an event from this function instead, so that we do
RB> this processing as a wx event?

Sorry, I'm confused here. The callback does write to the pipe. But we
only read from this pipe when our callback is called by GTK main loop.
Granted, this is not a wx event on its own, but it should be generated from
the same place/at the same time as GDK events which result in wx events. So
according to my understanding, gdk_display_flush() shouldn't result in our
callback being called. But my understanding could be wrong...

In any case, it doesn't hurt to register the PID immediately, of course.

[Rob Bresalier]

Vadim:

I've got this patch almost ready to go.  This is not my day job, so that is why its been a couple of weeks.

I've tested it on Mac and GTK (and I also verified nothing broken in Windows), and I am using a temporary event loop instead of polling now.  A little cleanup and adding some documentation, and I'll be ready to post it for review.

One question:  How to handle if the application terminates while child processes are still executing?  I will need to exit the temporary event loops running for any child processes if the application is terminating.  What should I use as a trigger to exit the temporary event loops?  I mean, I will need to find a place in the wx code where it has been decided that the application is going to terminate, so I will need to tell any temporary event loops running for child processes that they need to stop, even though the child process has not terminated yet.  So where to insert this code?

I'm not sure how well the existing code (polling) is handling this scenario.

Thanks,

Rob

[Vadim Zeitlin]

On Thu, 31 Jan 2013 00:55:00 -0500 Rob Bresalier wrote:

RB> I've got this patch almost ready to go. This is not my day job, so that is
RB> why its been a couple of weeks.

Hi,

I can totally understand this, thanks for taking time to bring this to
completion even so.

RB> One question: How to handle if the application terminates while child
RB> processes are still executing?

I think we should just leave them running. What else can we do?

RB> What should I use as a trigger to exit the temporary event loops? I mean,
RB> I will need to find a place in the wx code where it has been decided that
RB> the application is going to terminate, so I will need to tell any temporary
RB> event loops running for child processes that they need to stop, even though
RB> the child process has not terminated yet. So where to insert this code?

Sorry, I just don't remember what was the problem with the temporary event
loops. Where are they started from and why?

RB> I'm not sure how well the existing code (polling) is handling this scenario.

It just doesn't do anything and I think it's good enough...

[Rob Bresalier]

RB> What should I use as a trigger to exit the temporary event loops?  I mean,
RB> I will need to find a place in the wx code where it has been decided that
RB> the application is going to terminate, so I will need to tell any temporary
RB> event loops running for child processes that they need to stop, even though
RB> the child process has not terminated yet.  So where to insert this code?

 Sorry, I just don't remember what was the problem with the temporary event
loops. Where are they started from and why?

The problem that I was solving was that when we launch wxExecute with a child process in synchronous mode, we must wait for the process to complete before returning from wxExecute.  The existing code does polling, using wxYield in a loop.  You wrote in an earlier email that we should use a temporary event loop instead of polling:

You wrote about the current code's loop using wxYield:

"

 This loop is quite horrible too, of course. I have no idea why don't we
use a local event loop with the same self-pipe approach to handle IO. More
I look at this code, more I realize just how completely broken it is :-(

"

So my question was, if we are terminating the application, I need a way to exit the temporary event loop, also to handle the code after the event loop gracefully.  Is there already hooks in place in wxWidgets that when an application is terminating that all temporary event loops are exited?

RB> I'm not sure how well the existing code (polling) is handling this scenario.

 It just doesn't do anything and I think it's good enough...

If that is good enough then I guess we can just wait in the temp event loop too, just want to be sure.

[Vadim Zeitlin]

On Thu, 31 Jan 2013 20:06:22 -0500 Rob Bresalier wrote:

RB> The problem that I was solving was that when we launch wxExecute with a
RB> child process in synchronous mode,
[...explanation snipped...]

Ah, yes, I remember it now, thanks!

RB> So my question was, if we are terminating the application, I need a way to
RB> exit the temporary event loop, also to handle the code after the event loop
RB> gracefully. Is there already hooks in place in wxWidgets that when an
RB> application is terminating that all temporary event loops are exited?

It should be impossible for the application to exit while synchronous
execution is in process as all the window menus and other controls are
supposed to be disabled. And if this is not the case, i.e. if
wxApp::ExitMainLoop() is still called, somehow, then I think it simply
won't work before the child process terminates and the local loop ends
(although I'm not 100% sure about this, ExitMainLoop() might terminate the
local loop instead and not exit the application at all which would be a
problem...).

So I think we can disregard this problem for now and just assume that the
application doesn't exit during wxExecute(wxEXEC_SYNC) call. Of course, it
is perfectly possible that the application exits while some child processes
launched using wxExecute(wxEXEC_ASYNC) are still running, so it would be
worth verifying that calls to SIGCHLD handler at "inopportune" times (e.g.
in the middle of the application termination) don't result in any problems.
If they do, perhaps we should just unset the handler for this signal when
we start shutting down.

Thanks again,

[Rob Bresalier]

I finished developing my patch and I posted it to the Trac:

http://trac.wxwidgets.org/ticket/10258#comment:7

I would love to submit it to the review.bakefile.org, but I was having difficulty uploading the patch file there.  Maybe it needs a different format?  I could not get it to work.  If you have any tips for how to do it, I will gladly try again.

[Vadim Zeitlin]

On Tue, 5 Feb 2013 22:32:15 -0500 Rob Bresalier wrote:

RB> I finished developing my patch and I posted it to the Trac:
RB>
RB> http://trac.wxwidgets.org/ticket/10258#comment:7

Thanks, I'll try to look at it a.s.a.p.

RB> I would love to submit it to the review.bakefile.org, but I was having
RB> difficulty uploading the patch file there. Maybe it needs a different
RB> format? I could not get it to work. If you have any tips for how to do
RB> it, I will gladly try again.

Normally it should just work if you use "svn diff". You could also use
post-review (http://www.reviewboard.org/docs/manual/dev/users/tools/post-review/)
to create the review request easily.

Regards,

[Rob Bresalier]

Does review.bakefile.org take the same format patch file as the one that I uploaded to the Trac?

I had tried uploading that manually, but when I clicked on "Review" I did not see anything, it was blank.

I have not used svn before (I've used cvs and clearcase), so it would be a little learning curve.

Can I use the post-review without a version control system?  I did look at the website for it but I did not see any description of how to use it without having a version control system.

If I do use svn to upload, can I use post-review with just read only access to svn?

[Rob Bresalier]

Just for your reference, I created my patch file with this type of command (from Trac wiki):

diff -uNr wxWidgets-orig wxWidgets-mine > mypatch.patch

Is this type of format what I can upload to review-board?

[Vadim Zeitlin]

On Wed, 6 Feb 2013 07:21:43 -0500 Rob Bresalier wrote:

RB> Does review.bakefile.org take the same format patch file as the one that I
RB> uploaded to the Trac?

I'm not sure. I think it might want patches without the leading parent
directory, i.e. applicable with "patch -p0", not -p1.

RB> I had tried uploading that manually, but when I clicked on "Review" I did
RB> not see anything, it was blank.

This is not normal, it should give an error if the patch format is
incorrect.

RB> I have not used svn before (I've used cvs and clearcase), so it would be a
RB> little learning curve.

FWIW if you want to learn a new VCS, I'd start with git and not svn. The
instructions for starting to use it:

0. Install git and RBTools.
1. Run "git clone git://github.com/wxWidgets/wxWidgets.git" (if you're
behind a firewall, you can use https://github.com/wxWidgets/wxWidgets.git)
2. Run "git checkout -b wxexec-fixes" (branch name is whatever you'd like).
3. Run "git apply -p1 path/to/your/execute_patch.patch".
4. Run "git commit -a" to commit these changes.
5. Run "git config reviewboard.url http://review.bakefile.org".
6. Run "post-review --guess-summary --guess-description
--target-people=zeitlin".
7. Go to the URL printed by the previous step, check that everything looks
correctly, fill in the missing fields (e.g. "Testing done") and press
"Submit" button.

RB> Can I use the post-review without a version control system?

I don't think so, at least I've only ever used it with VCS (either svn or
git).

RB> If I do use svn to upload, can I use post-review with just read only access
RB> to svn?

Yes, this should work.

[Rob Bresalier]

Thanks for your notes.  I tried it, but at the last step I get this error message:

ubuntu:/home/rob/wx/wxgit/wxWidgets

=> post-review --guess-summary --guess-description --target-people=zeitlin

==> HTTP Authentication Required

Enter authorization information for "Web API" at review.bakefile.org

Username: rbresalier

Password: 

Error creating review request: One or more fields had errors (HTTP 400, API Error 105)

[Vadim Zeitlin]

On Wed, 6 Feb 2013 08:00:10 -0500 Rob Bresalier wrote:

RB> ubuntu:/home/rob/wx/wxgit/wxWidgets
RB> => post-review --guess-summary --guess-description --target-people=zeitlin
RB> ==> HTTP Authentication Required
RB> Enter authorization information for "Web API" at review.bakefile.org
RB> Username: rbresalier
RB> Password:
RB> Error creating review request: One or more fields had errors (HTTP 400, API
RB> Error 105)

Hmm, this is really strange. I've only seen errors from post-review if I
was using an out of date version of it but if you've just installed RBTools
it should be fine. Still, just because I can't think of anything else, what
does "post-review --version" say?

You could also try updating the (currently empty) review request by
appending "-r 469" to the command line to see if this wasn't a one off
error...

[Rob Bresalier]

Here is my output of "post-review --version":

ubuntu:/home/rob/wx/wxgit/wxWidgets

=> post-review --version

RBTools 0.4.3

I tried -r 469, and got this:

ubuntu:/home/rob/wx/wxgit/wxWidgets

=> post-review --guess-summary --guess-description --target-people=zeitlin -r 469

Error getting review request 469: One or more fields had errors (HTTP 400, API Error 105)

[Vadim Zeitlin]

On Wed, 6 Feb 2013 09:40:43 -0500 Rob Bresalier wrote:

RB> Here is my output of "post-review --version":
RB>
RB> ubuntu:/home/rob/wx/wxgit/wxWidgets
RB> => post-review --version
RB> RBTools 0.4.3

The same as mine (and latest AFAIK).

RB> I tried -r 469, and got this:
RB>

RB> ubuntu:/home/rob/wx/wxgit/wxWidgets
RB> => post-review --guess-summary --guess-description --target-people=zeitlin

RB> -r 469
RB> Error getting review request 469: One or more fields had errors (HTTP 400,
RB> API Error 105)

I really have no idea what's going on here :-( Maybe you should retry
creating a new request again (without the "-r" option) and use "-d" option
to see debugging information which might give a clue to what goes wrong.

[Rob Bresalier]

OK, here is the output:

ubuntu:/home/rob/wx/wxgit/wxWidgets

=> post-review --guess-summary --guess-description --target-people=zeitlin -d

>>> RBTools 0.4.3

>>> Python 2.6.5 (r265:79063, Oct  1 2012, 22:07:21) 

[GCC 4.4.3]

>>> Running on Linux-2.6.32-45-generic-i686-with-Ubuntu-10.04-lucid

>>> Home = /home/rob

>>> Current Directory = /home/rob/wx/wxgit/wxWidgets

>>> Checking the repository type. Errors shown below are mostly harmless.

DEBUG:root:Checking for a Bazaar repository...

DEBUG:root:Checking for a CVS repository...

DEBUG:root:Checking for a ClearCase repository...

DEBUG:root:Checking for a Git repository...

DEBUG:root:Running: git rev-parse --git-dir

DEBUG:root:Running: git config core.bare

DEBUG:root:Running: git rev-parse --show-toplevel

DEBUG:root:Running: git symbolic-ref -q HEAD

DEBUG:root:Running: git config --get branch.wxexec-fixes.merge

DEBUG:root:Command exited with rc 1: ['git', 'config', '--get', 'branch.wxexec-fixes.merge']

---

DEBUG:root:Running: git config --get branch.wxexec-fixes.remote

DEBUG:root:Command exited with rc 1: ['git', 'config', '--get', 'branch.wxexec-fixes.remote']

---

DEBUG:root:Running: git config --get remote.origin.url

DEBUG:root:repository info: Path: git://github.com/wxWidgets/wxWidgets.git, Base path: , Supports changesets: False

>>> Finished checking the repository type.

DEBUG:root:Running: git config --get reviewboard.url

>>> HTTP GETting api/info/

DEBUG:root:Running: git merge-base origin/master refs/heads/wxexec-fixes

DEBUG:root:Running: git diff --no-color --full-index --no-ext-diff --ignore-submodules --no-renames e949d38c4e59441c90c1332889005b46b2d36f63..refs/heads/wxexec-fixes

DEBUG:root:Running: git log --pretty=format:%s HEAD^..

DEBUG:root:Running: git log --pretty=format:%s%n%n%b e949d38c4e59441c90c1332889005b46b2d36f63..

>>> HTTP GETting api/

>>> HTTP GETting http://review.bakefile.org/api/info/

>>> Using the new web API

>>> Attempting to create review request on git://github.com/wxWidgets/wxWidgets.git for None

>>> HTTP POSTing to http://review.bakefile.org/api/review-requests/: {'repository': 'git://github.com/wxWidgets/wxWidgets.git'}

>>> Review request created

>>> Attempting to set field 'target_people' to 'zeitlin' for review request '470'

>>> HTTP PUTting to http://review.bakefile.org/api/review-requests/470/draft/: {'target_people': 'zeitlin'}

>>> Got API Error 105 (HTTP code 400): One or more fields had errors

>>> Error data: {u'fields': {u'target_people': [u'zeitlin']}, u'stat': u'fail', u'draft': {u'last_updated': u'2013-02-06 16:47:07', u'description': u'', u'links': {u'self': {u'href': u'http://review.bakefile.org/api/review-requests/470/draft/', u'method': u'GET'}, u'update': {u'href': u'http://review.bakefile.org/api/review-requests/470/draft/', u'method': u'PUT'}, u'draft_screenshots': {u'href': u'http://review.bakefile.org/api/review-requests/470/draft/screenshots/', u'method': u'GET'}, u'draft_file_attachments': {u'href': u'http://review.bakefile.org/api/review-requests/470/draft/file-attachments/', u'method': u'GET'}, u'review_request': {u'href': u'http://review.bakefile.org/api/review-requests/470/', u'method': u'GET', u'title': u'(no summary)'}, u'delete': {u'href': u'http://review.bakefile.org/api/review-requests/470/draft/', u'method': u'DELETE'}}, u'changedescription': u'', u'target_groups': [], u'bugs_closed': [], u'target_people': [], u'testing_done': u'', u'branch': u'', u'id': 8, u'summary': u'', u'public': False}, u'err': {u'msg': u'One or more fields had errors', u'code': 105}}

Error creating review request: One or more fields had errors (HTTP 400, API Error 105)

ubuntu:/home/rob/wx/wxgit/wxWidgets

=> 

[Vadim Zeitlin]

On Wed, 6 Feb 2013 10:48:18 -0500 Rob Bresalier wrote:

RB> OK, here is the output:

OK, looks like I made the mistake in the only parameter which I don't
use... the name should be "VZ" probably and not "zeitlin", sorry. If in
doubt, please try without --target-people switch completely and then choose
the reviewer interactively on the web page.

Thanks,

[Rob Bresalier]

OK, I think that worked.

But when I click on the "Review" button, all I get is an edit box to add a comment. 

How do I see the code that should be reviewed?

[Vadim Zeitlin]

On Wed, 6 Feb 2013 11:07:25 -0500 Rob Bresalier wrote:

RB> OK, I think that worked.

Yes, it did, thanks, and sorry for the wrong instructions.

RB> But when I click on the "Review" button, all I get is an edit box to add a
RB> comment.
RB>
RB> How do I see the code that should be reviewed?

You need to click "View diff" button for this. Then you can click on any
line (or also select a range of lines) to enter a comment for it/them.
Notice that the comments are not published until you submit them!

Regards,

[Rob Bresalier]

Thanks!  I see it now.

Well, happy reviewing!  I look forward to reading your comments.  Hopefully you have the time to do it.

[Rob Bresalier]

I've created a new patch that addresses all of the issues raised by the code review of the first patch located here:

http://review.bakefile.org/r/471/

In this second patch, all changes to wxFile are removed, and only a Detach() function has been added to wxFileInputStream and wxInputStream. (All other changes to wxFileInputStream and wxInputStream from the first patch have been removed).

All trivial accessor functions have been changed to 'const', with a few important exceptions (because of compiler errors), and are noted in comments in the code.

All 'void' pointers that were present in the first patch have been removed and replaced with the appropriate types.

All style issues have been corrected (spaces added where needed, trailing whitespace removed).  I believe I caught every place where there was a style issue, but if I missed any then please comment.

The patch is against the 2.9.4 release as I had issues compiling a recent trunk on Linux.  The patch has been tested on OSX, GTK, and Windows.  The test suite has been run and no new errors were introduced vs the 2.9.4 baseline.

I also added 'exec.cpp' to the GUI test suite in tests.bkl (in addition to already being in the console test suite), and I added a few more tests as well (see exec.cpp).

Looking forward to the comments from the second review.

An "execution roadmap" has been created to make this review easier.  If you perform the review in the order specified by the "Execution Roadmap", the review will be easier to follow and understand.

Execution Roadmap:
==================

Notes:
When you see a 'see below' comment as follows:
// See below
It means the code is also reused in other places and its call tree will be shown
later in the roadmap.

wxAppConsole::wxAppConsole()            // appunix.cpp
{
    Initialize m_wakeupPipe to NULL
} // End wxAppConsole::wxAppConsole()
    
wxExecute()                             // utilsunx.cpp
{
    wxApp::SetSignalHandler()           // appunix.cpp
    WakeUpPipe::WakeUpPipe()            // executepipe.h
    WakeUpPipe::Create()                // utilsunx.cpp

    // *** Register the callback function for child termination ***
    // *** Same code reused in stdout/stderr callback registration. ***
    wxAppTraits::RegisterProcessCallback(wxExecuteCallbackPipe *data, int fd)  // utilsunx.cpp
    {
        // AddProcessCallback is platform specific, one of:
        (console) wxAppTraits::AddProcessCallback()     // utilsunx.cpp
        (OSX)     wxGUIAppTraits::AddProcessCallback()  // utilsexc_cf.cpp
        (GTK2)    wxGUIAppTraits::AddProcessCallback()  // src/gtk/utilsgtk.cpp
        (GTK1)    wxGUIAppTraits::AddProcessCallback()  // src/gtk1/utilsgtk.cpp
        (Motif)   wxGUIAppTraits::AddProcessCallback()  // utils.cpp
    } // end RegisterProcessCallback()

    wxUpdateListOfOpenChildProcesses()    // utilsunx.cpp
    wxEndProcessData::wxEndProcessData()  // execute.h
    wxProcess::SetEndProcDataPtr()        // include/wx/process.h
    wxProcess::Redirect(true)             // include/wx/process.h
    wxProcess::SetBufOut()                // include/wx/process.h
    wxProcess::SetBufErr()                // include/wx/process.h
    wxStreamTempInputBuffer::Init()       // execcmn.cpp
    wxProcess::IsNotifyEnabled()          // include/wx/process.h
    wxExecuteIOHandler::Init()            // executepipe.h

    // *** Register the callback function for stdout/stderr ***
    // *** Same code reused in child process termination callback registration ***
    wxAppTraits::RegisterProcessCallback(wxExecuteCallbackPipe *data, int fd)  // utilsunx.cpp
    {
        // AddProcessCallback is platform specific
        (console) wxAppTraits::AddProcessCallback()     // utilsunx.cpp
        (OSX)     wxGUIAppTraits::AddProcessCallback()  // utilsexc_cf.cpp
        (GTK2)    wxGUIAppTraits::AddProcessCallback()  // src/gtk/utilsgtk.cpp
        (GTK1)    wxGUIAppTraits::AddProcessCallback()  // src/gtk1/utilsgtk.cpp
        (Motif)   wxGUIAppTraits::AddProcessCallback()  // utils.cpp
    }

    (Console app) wxAppTraits::WaitForChild(wxExecuteData& execData) // utilsunx.cpp
    {
        wxAppTraits::WaitForChildSync() // See below - common with GUI
    }
    (GUI app)     wxGUIAppTraits::WaitForChild(wxExecuteData& execData) // apptraits.cpp
    {
        wxAppTraits::WaitForChildSync() // See below - common with Console
    }

    // WaitForChildSync is common to GUI and Console
    wxAppTraits::WaitForChildSync(wxExecuteData& execData) // utilsunx.cpp
    {
        // **** EVENT LOOP FOR SYNCHRONOUS WXEXECUTE RUNS HERE ***
        Event Loop
        {
            // SIGCHLD Signal Interrupt Handling    
            (SIGCHLD) wxAppConsole::HandleSignal()  // appunix.cpp
            {
                wxAppConsole::WakeUp()        // appunix.cpp
                {
                    WakeUpPipe::WakeUpNoLock()  // utilsunx.cpp
                }
            }

            // Platform Specific Callback Handlers:

            // Console callbacks on stdout/stderr/termination pipe reception
            (Console-child termination)   WakeUpPipe::OnReadWaiting() // See below
            (Console-stdout/stderr avail) wxExecuteIOHandler::OnReadWaiting() // See below

            // GUI callbacks on stdout/stderr/termination pipe reception
            (OSX-GUI)                     WXCF_OnReadWaiting()        // utilsexc_cf.cpp
            (GTK-GUI)                     GTK_OnReadWaiting()         // src/gtk1/utilsgtk.cpp
            (GTK1-GUI)                    GTK1_EndProcessDetector()   // src/gtk1/utilsgtk.cpp
            (Motif-GUI)                   xt_OnReadWaiting()          // utils.cpp

            // wxOnReadWaiting() is called by all GUI callbacks (and not by console callbacks)
            wxOnReadWaiting()  // appunix.cpp
            {
                wxAppTraits::IsFdCallbackEnabled()   // utilsunx.cpp
                wxAppTraits::GetFdExecutePipe()      // utilsunx.cpp)   
                (child termination) WakeUpPipe::OnReadWaiting() // see below
                (stdout/stderr)     wxExecuteIOHandler::OnReadWaiting() // see below
            } // End of wxOnReadWaiting()

            // stdout/stderr handling - Common for GUI and Console
            wxExecuteIOHandler::OnReadWaiting()  // utilsunx.cpp
            {
                wxProcess::GetEndProcDataPtr()  // include/wx/process.h
                wxStreamTempInputBuffer::Eof()  // execcmn.cpp
                wxProcess::OnInputAvailable()  // include/wx/process.h - async only
                wxProcess::OnErrorAvailable()  // include/wx/process.h - async only
                wxExecuteIOHandler::DisableCallback()  // utilsunx.cpp
                {
                    wxExecuteCallbackPipe::IsShutDownFlagSet()  // executepipe.h
                    wxAppTraits::UnRegisterProcessCallback()    // utilsunx.cpp
                    {
                        // Platform Specific RemoveProcesscallback():
                        (console app)  wxAppTraits::RemoveProcesscallback()  // utilsunx.cpp
                        (OSX)          wxGUIAppTraits::RemoveProcesscallback()  // utilsexc_cf.cpp
                        (GTK2)         wxGUIAppTraits::RemoveProcesscallback()  // src/gtk/utilsgtk.cpp
                        (GTK1)         wxGUIAppTraits::RemoveProcesscallback()  // src/gtk1/utilsgtk.cpp
                        (Motif)        wxGUIAppTraits::RemoveProcesscallback()  // utils.cpp
                    }
                    wxFileInputStream::Detach()  // wfstream.h, stream.h
                    wxExecuteCallbackPipe::SetShutDownFlag()  // executepipe.h
                }
                wxEndProcessData::CheckHandleTermination() // see below, common with child termination
            } // wxExecuteIOHandler::OnReadWaiting()

            // Child Termination handling - Common for GUI and console
            WakeUpPipe::OnReadWaiting() // utilsunx.cpp
            {
                wxAppConsole::OnWakeUp()  // appunix.cpp
                {
                    wxAppConsole::CheckSignal()  // appunix.cpp
                    {
                        wxCheckChildProcessTermination()  // utilsunx.cpp
                        {
                            DoWaitForChild()  // utilsunx.cpp
                            {
                                // CheckHandleTermination is common with stdout/stderr
                                wxEndProcessData::CheckHandleTermination() // see below
                            } // DoWaitForChild()
                        } // wxCheckChildProcessTermination()
                    } // wxAppConsole::CheckSignal()
                } // wxAppConsole::OnWakeUp()
            } // WakeUpPipe::OnReadWaiting()

            // CheckHandleTermination() is called at pipe closure for each stdout/stderr/termination pipe
            wxEndProcessData::CheckHandleTermination() // utilsunx.cpp
            {
                wxExecuteCallbackPipe::IsShutDownFlagSet()  // executepipe.h
                wxHandleProcessTermination()  // utilsunx.cpp
                {
                    wxProcess::OnTerminate() // async processes only

                    *** Event Loop is told to stop here ***

                } // wxHandleProcessTermination
            } // CheckHandleTermination()
        } // End of Event loop
    } // end wxAppTraits::WaitForChildSync(wxExecuteData& execData) // utilsunx.cpp
            
    Allocate wxMemoryStream from temp buffers m_bufOut/m_bufErr
    wxProcess::ReplaceInputStream()  // utilsunx.cpp
    wxProcess::ReplaceErrorStream()  // utilsunx.cpp

    wxStreamTempInputBuffer::~wxStreamTempInputBuffer() // Windows only - execcmn.cpp
    {
        wxFileInputStream::Ungetch()  // Windows only, NOT Unix.
    }
} // End of wxExecute()

wxProcess::~wxProcess()  // process.cpp
{
    delete m_stdoutHandler and m_stderrHandler
    delete m_bufOut, m_bufErr, 
    {
        wxStreamTempInputBuffer::~wxStreamTempInputBuffer()
    }
}
    
wxAppConsole::~wxAppConsole()
{
    delete m_wakeupPipe
}
    
Tests
{
    test.bkl:  Added exec.cpp for GUI also
    exec.cpp:  Added some new tests for wxExecute-sync-stderr and wxExecute-sync
}

[Vadim Zeitlin]

On Fri, 15 Feb 2013 20:49:59 -0500 Rob Bresalier wrote:

RB> I've created a new patch that addresses all of the issues raised by
RB> the code review of the first patch located here:
RB>
RB> http://review.bakefile.org/r/471/

Hi,

Thanks for all your effort, I'll try to look at it a.s.a.p. but I've got
absolutely no time right now and probably will only able to do it the next
week-end. Right now I can't even read this email fully, but just wanted to
tell you that I do see your review/ticket updates and emails and will get
to them as soon as it becomes physically possible.

Sorry again for the delay,