Nested Event Loops

[Rob Bresalier]

Hi:

This message is for explaining code that was written in the patch for wxExecute that allows nested event loops to have a working implementation.

See:

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

and

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

In the review comment Vadim asked me to take this to wx-dev.

We can debate whether or not nested event loops should be supported or not.

But if you think it should be supported (which I am in favor of), then I can explain how it can be made to work in a low complexity way.  It can be risky to nest event loops, but if a programmer is careful it can be made to work.  I've also found that the Webkit source code allows nested event loops.

Additionally, the test case for the nested scenario described below has been written and it passes on GTK, Cocoa, and Carbon.

See this change in src/gtk/evtloop.cpp in execute_patch5.patch from the Trac:

    // This is placed inside of a loop to take into account nested
    // event loops.  For example, inside this event loop, we may recieve
    // Exit() for a different event loop (which we are currently inside of)
    // That Exit() will cause this gtk_main() to exit so we need to re-enter it.
    while ( !m_shouldExit )
    {
        gtk_main();
    }

    // Force the enclosing event loop to also exit to see if it is done
    // in case that event loop had Exit() called inside of the just ended loop.
    //  If it is not time yet for that event loop to exit, it will be executed 
    // again due to the while() loop on m_shouldExit().
    //
    // Don't call gtk_main_quit() if we are not inside a nested event loop, so
    // check loopLevel first.
    if (loopLevel != 0)
    {
        gtk_main_quit();
    }

For some background, this patch changed the wxExecute(SYNC) code to use an event loop while waiting for the process to terminate instead of the previous code using wxYield.

Now, consider the scenario of 2 nested event loops due to wxExecute(SYNC) in GTK:

1. wxExecute(Process A, sync)

2. event loop A ( starts with call to gtk_main() )

3. wxExecute(Process B, sync)

4. event loop B  (another call to gtk_main()  )

5.  Process A completes, signals that event loop A should be exited (wxGuiEventLoop::Exit() call).

  5a. The code that signals event loop termination can be simple without having knowledge of which event loop it is in.  So it can simply call gtk_main_quit() without complex code to determine if it is in a nested event loop or not.

  5b.In addition to calling gtk_main_quit(), it also sets its m_shouldExit flag (which is specific to event loop A's object) so that event loop A knows if it really needs to exit.

6. Since we called gtk_main_quit(), event loop B's gtk_main() call will exit, however we still want to be inside of event loop B because process B has not finished yet.  Since the m_shouldExit flag is not set yet for event loop B, we will re-enter event loop B, which is what we desire to do - which will not get exited until Process B termination is detected and it calls Exit() for event loop B.

7. Process B terminates, and calls gtk_main_stop().

7a.  This only causes event loop B's gtk_main() to be exited.  event loop A's gtk_main() will not automatically get exited.

7b. To give event loop A an opportunity to exit if it needs to, we make the call to gtk_main_quit() (only if we are not in the main event loop - that is  (loopLevel != 0) ), so that event loop A can exit, and check if it should get re-entered by checking its m_shouldExit flag in the 'while(!m_shouldExit)" loop.

Similar changes are made in the Cocoa and Carbon event loops to loop on m_shouldExit for the same reasons.

Additionally, you can see that this concept of looping on m_shouldExit is already used in wxEventLoopManual::Run() even before my patch was written.

Comments are welcomed!
Rob Bresalier

[Vadim Zeitlin]

On Sun, 21 Apr 2013 23:40:55 -0400 Rob Bresalier wrote:

RB> This message is for explaining code that was written in the patch for
RB> wxExecute that allows nested event loops to have a working implementation.

Hi Rob,

Thanks for the explanation!

RB> We can debate whether or not nested event loops should be supported or not.

I think the main problem is not the nested event loops, which we already
support as you can have nested modal dialogs, but the fact that the outer
loop can be asked to terminate before the inner event loop does. This makes
them not really nested but rather overlapping which is quite a bit more
complex...

RB> Additionally, you can see that this concept of looping on m_shouldExit
RB> is already used in wxEventLoopManual::Run() even before my patch was
RB> written.

This is quite different, it only involves a single loop and the loop there
is very simple to understand, it's literally just

while (!m_shouldExit) {
DispatchEvents();
if ( ExitRequested() )
m_shouldExit = true;
}

nothing more. This is as simple as a message loop can be, basically. While
the new code involves a complex interplay between two (or more!) event
loops and while I do understand how it works now, thanks to your
explanation, and can even admire the elegance, it's hardly simple at all.
It also relies on some assumptions about how the event loops work, e.g.
that you can exit the current event loop without knowing which one it is
and that you can restart the event loop after it terminates, that might not
be true everywhere even if they seem to work for all platforms we support
right now.

RB> Comments are welcomed!

My preferred solution would be keep things maximally simple and just say
that wxExecute(SYNC) is not reentrant and live with it. Provided that this
is detected by the library, i.e. results in a clear assert failure message,
and is documented, I think it's a reasonable limitation. In fact I'd really
prefer to deprecate wxExecute(SYNC) completely, all our experience shows
that emulating synchronous execution is just not worth it, so you should
use either wxExecute(ASYNC) for long running child processes, or
wxExecute(BLOCK) for short running ones.

If we absolutely must have support for reentrant/recursive calls to
wxExecute(SYNC), then I'd like to at least avoid making the base classes
such as wxEventLoop more complex and keep all this logic inside wxExecute()
itself. It should be possible to do it by keeping track of which loop
corresponds to each process. I.e., continuing your example with

wxExecute(A) -> loop A -> wxExecute(B) -> loop B

we should only exit the loop B when the process B terminates and if the
process A terminates while loop B is running, we need to remember it and
exit the loop A just before returning from wxExecute(B).

What do you think?
VZ

[Rob Bresalier]

Hi Vadim:

VZ> I think the main problem is not the nested event loops, which we already

VZ> support as you can have nested modal dialogs, but the fact that the outer
VZ> loop can be asked to terminate before the inner event loop does. This makes
VZ> them not really nested but rather overlapping which is quite a bit more
VZ> complex...

I agree with this assessment.  It is the fact that the outer loop is asked to terminate before the inner loop does that is the main issue we are discussing.

While it makes the event loop code a little more complex (I really don't think it is that bad), I think it makes the library user's life much more simple.  He does not need to worry about whether or not he is in an inner or outer loop when he tries to exit his loop (if he is using different, unrelated code that has temporary event loops).  He does not have to write complex code to make sure he does not end an outer loop inside an inner loop - he does not even need to be aware of that.

I think that enabling the usage of overlapped loops makes the end user's (user of event loop classes, not just wxExecute) life much simpler.  I have proven it can be done.  And I think the purpose of a library is to make a programmer's job simpler, so that the programmer does not need to spend time solving problems that the library developers already solved for them.

As I think this is a general issue with event loops, whether or not it is used by wxExecute(SYNC) or not, I think the proper place to solve this problem is in the event loop classes.  If overlapped event loops are needed or desired for wxExecute (and I say, why not, let the user have the ability to do it), I think your proposal to put the code inside of wxExecute to have knowledge of when it can tell which event loop to exit is more complex than the code that I put into the event loop classes to handle overlapping.  Furthermore, if the end user of the library wants to use wxGuiEventLoop in his code, then this problem could already be solved for him if my trick is in the wx event loop classes, then he does not need to worry about it.  We can just solve this once for everyone by doing it in the event loop classes in wx.

BTW: The wx implementation in 2.9.4 (without my patch) of just 1 temporary event loop (not the modal dialog one) called from somewhere in the main loop is broken in the 2.9.4 release.  It is because of the Cocoa API, you use [NSApp run] to start the loop, and [NSApp stop] to end the loop.  But I have found out from testing that [NSApp stop] will not only cause the inner event loop to terminate, it will also cause the main loop to exit!  Another undesired result of calling [NSApp stop] is that it can also cause a modal dialog event loop to also exit (which would be undesired).  See my changes and extensive comments in evtloop.mm for cocoa for how I solved this issue (reusing some tricks from webkit source code + adding my own).  This solution also allows overlapped event loops.

VZ> While the new code involves a complex interplay between two (or more!) event
VZ> loops and while I do understand how it works now, thanks to your
VZ> explanation, and can even admire the elegance, it's hardly simple at all.
VZ> It also relies on some assumptions about how the event loops work, e.g.
VZ> that you can exit the current event loop without knowing which one it is
VZ> and that you can restart the event loop after it terminates, that might not
VZ> be true everywhere even if they seem to work for all platforms we support
VZ> right now.

Each place where this trick (and I agree it is elegant :) ) is implemented has platform specific code.  There is a specific version for GTK, a specific version for Carbon, and a specific version for Cocoa.  In fact in the Cocoa implementation this trick is used in a slightly different way - you can see the extensive comments in evtloop.mm about it.

All 3 of these platforms have been proven to work, you can try building the tests with my patch, and see it for yourself in the ExecTestCase tests.

You can see this page from the GTK API that clearly says it is supported by the definition of the gtk_main() and gtk_main_quit() APIs:

http://www.gtk.org/api/2.6/gtk/gtk-General.html#gtk-main

So as this is the spec of gtk_main()/gtk_main_quit() - it will work for all GTKs on any Unix platform.

For carbon/cocoa I've proven it works on 10.6 - so it should work on all later versions of OSX as well.

So if the trick has been shown to work on GTK, carbon and cocoa, then why do you think that it would not work everywhere?  Where do you think it wouldn't work?

Regards,

Rob

[Vadim Zeitlin]

On Mon, 22 Apr 2013 23:00:35 -0400 Rob Bresalier wrote:

RB> As I think this is a general issue with event loops, whether or not it is
RB> used by wxExecute(SYNC) or not, I think the proper place to solve this
RB> problem is in the event loop classes.

I agree that the changes should be done to wxEventLoop itself if they're
general and are not only needed in order to make wxExecute(SYNC) work.
However I'm not sure how would they be used, in general. I.e. what is our
API here? It seems we need some way to say "exit from this loop" instead of
just being able to terminate the currently running loop, which is all that
we currently have.

Moreover, we probably also need "exit from all the event loops up and
including this one" operation too. This would be mostly useful for the main
loop itself, i.e. currently wxApp::ExitMainLoop() doesn't do anything if
the main loop is not running. It would probably make more sense if it
exited from all the currently running loops and really terminated the
application in all situations. And it seems like your changes should allow
doing this easily -- but there is no API for this currently, is there?

So what do you think about defining this API for wxEventLoop precisely?
Would it be possible for you to do it, at least at the email level,
ensuring that this API is both

(a) Enough to make wxExecute(SYNC) implementation work.

and

(b) General enough to be useful in other situations.

?

Of course, if you can do it at the code level too (i.e. make a patch with
these changes and the associated documentation/tests updates), it would be
perfect, but if we can at least fix the API together, then I should be able
to extract the relevant parts of your patch myself, hopefully...


RB> BTW: The wx implementation in 2.9.4 (without my patch) of just 1 temporary
RB> event loop (not the modal dialog one) called from somewhere in the main
RB> loop is broken in the 2.9.4 release. It is because of the Cocoa API, you
RB> use [NSApp run] to start the loop, and [NSApp stop] to end the loop. But I
RB> have found out from testing that [NSApp stop] will not only cause the inner
RB> event loop to terminate, it will also cause the main loop to exit! Another
RB> undesired result of calling [NSApp stop] is that it can also cause a modal
RB> dialog event loop to also exit (which would be undesired). See my changes
RB> and extensive comments in evtloop.mm for cocoa for how I solved this issue
RB> (reusing some tricks from webkit source code + adding my own).

Thanks, I'll try to have a look at this a bit later.

BTW, not saying that it necessarily needs to be done now, but this is just
an example of things that probably should have factored out of the main
patch: as the behaviour described above is clearly a bug, it would have
been great to have a commit fixing just it. Then another commit adding
nested/overlapped event loops support. And then the rest. Again, this is
not a criticism, just an explanation of how the changes could be organized
(and should be, if you make another patch in the future -- or, in fact, if
anybody else reading this, does) to make applying them much simpler.


RB> So if the trick has been shown to work on GTK, carbon and cocoa, then why
RB> do you think that it would not work everywhere? Where do you think it
RB> wouldn't work?

Well, we'd obviously need to make it work under MSW if this is to be an
officially supported part of the API. I think it shouldn't be a problem
there as MSW event loop API is pretty flexible (much more so than GTK or
Cocoa one), but I didn't look at the details of implementing it there yet.
Did you?

Thanks,
VZ

[Rob Bresalier]

Hi Vadim:

VZ> It seems we need some way to say "exit from this loop" instead of
VZ> just being able to terminate the currently running loop, which is all that
VZ> we currently have.

In fact, we already do have this API to say "exit from this particular event loop".  Also, we don't have an API to say "terminate the currently running loop".

In our example of

Process A->event loop A->Process B->event loop B

Both event loop A and event loop B are using different wxGuiEventLoop instantiations.  Now there is a static element inside that class that keeps track of the nesting, but we still have 2 different instantiations, but most of the member data is repeated for each instance and is not static.  

So each of these instances has their own 'm_shouldExit' flag.  So when telling event loop A to exit, we are using event loop A's dedicated instance as the object (that is we call eventLoopA->Exit()), and for telling event loop B to exit, we are using event loop B's dedicated instance (that is we call eventLoopB->Exit()).

It is only internal to the event loop class itself where we exit the current event loop as the 'trick' for implementing the overlapping, but we will go right back into it (due to the while(!m_shouldExit) if that was not the loop that we desired to exit.  As far as the user of the library user is concerned, he is only telling event loop A or B to exit, and not saying to exit the current one (exiting the current one is only something internal to the library as part of the 'trick').  In fact, there is no way to say "exit the current loop", as you must provide an instance of the wxGuiEventLoop object to call Exit().

VZ> Moreover, we probably also need "exit from all the event loops up and
VZ> including this one" operation too.

I think this could easily be implemented.  Here is how I would propose to do it:

1) When we run each new loop (call to ::Run() method), we add to a static hashmap of the event loop class a pointer to the object representing the event loop.  Similarly, we remove the object when exiting the loop.

2) We implement the "ExitFromAllLoops" as a static function of one of the base classes of wxGuiEventLoop (using a base class allows the code to be reused across platforms).  Then, this function simply calls the "Exit()" of each event loop that we've stored in the hash map.

VZ> BTW, not saying that it necessarily needs to be done now, but this is just
VZ> an example of things that probably should have factored out of the main
VZ> patch: as the behaviour described above is clearly a bug, it would have
VZ> been great to have a commit fixing just it. Then another commit adding
VZ> nested/overlapped event loops support. And then the rest. Again, this is
VZ> not a criticism, just an explanation of how the changes could be organized
VZ> (and should be, if you make another patch in the future -- or, in fact, if
VZ> anybody else reading this, does) to make applying them much simpler.

I perfectly understand your point here.  All I will say is that these issues were found while I was testing this patch, so it was natural to include them here to test that the issues were overcome.  Plus, I did not know you would even like this change.  They could be separated into their own commits if needed.

VZ> Well, we'd obviously need to make it work under MSW if this is to be an
VZ> officially supported part of the API. I think it shouldn't be a problem
VZ> there as MSW event loop API is pretty flexible (much more so than GTK or
VZ> Cocoa one), but I didn't look at the details of implementing it there yet.
VZ> Did you?

I remember looking at it, and I can't research it at this moment (at my day job), but I can look at it in more detail tonight.  But from what I recall your assessment is correct.  The issue with GTK, Carbon and Cocoa is that they have a function that encapsulates the loop - that is checking/dispatching each event and then checking if that loop should exit - this makes it much more difficult.  But on MSW I think it is different, we implement that loop ourselves in the wx library.  So we have better control of when we can exit the event loop.

In fact, when you look at the cocoa implementation that I did, you can see that we actually can't call [NSApp run] recursively, and the solution there was to re-implement the event loop ourselves by checking/dispatching events and implementing our own loop deciding when to exit.  So when we have our own loop doing this, it is much easier to do the overlapping trick.

Regards,

Rob

[Vadim Zeitlin]

On Tue, 23 Apr 2013 10:11:13 -0400 Rob Bresalier wrote:

RB> VZ> It seems we need some way to say "exit from this loop" instead of
RB> VZ> just being able to terminate the currently running loop, which is all
RB> VZ> that we currently have.
RB>
RB> In fact, we already do have this API to say "exit from this particular
RB> event loop". Also, we don't have an API to say "terminate the currently
RB> running loop".

We do, it's wxEventLoop::GetActiveLoop()->Exit(). You're right that
formally wxEventLoop::Exit() does correspond to my "exit from this loop"
operation but in practice it doesn't behave correctly in presence of
multiple loops and you can only call it on the innermost loop, so currently
you always must call it on wxEventLoop::GetActiveLoop() or the results risk
to be very surprising. And, in fact, Exit() asserts that the loop is
running exactly because of this, so in practice Exit() is the "exit the
currently running loop" operation and we do not have the other one.

Thinking more about this, it probably isn't a good idea to provide such
operation anyhow, though. What does it mean to exit from the outer loop
while the inner loop continues to run? It can't be even defined correctly
with the traditional C stack model. So Exit() should probably remain
unchanged, including the assert inside it.

RB> VZ> Moreover, we probably also need "exit from all the event loops up and
RB> VZ> including this one" operation too.

... and the only addition is this operation.

RB> I think this could easily be implemented.

Before discussing the implementation, I was really mostly asking about the
API, i.e. the interface. Again, I didn't have time to think about it much
yet but we basically have a choice between changing the existing Exit() to
behave like this or adding a new ExitAllNestedAndThisLoop() method.
Personally I prefer the latter (except that we need to find a better name
for this one).

RB> Here is how I would propose to do it:
RB>
RB> 1) When we run each new loop (call to ::Run() method), we add to a static
RB> hashmap of the event loop class a pointer to the object representing the
RB> event loop. Similarly, we remove the object when exiting the loop.
RB>
RB> 2) We implement the "ExitFromAllLoops" as a static function of one of the
RB> base classes of wxGuiEventLoop (using a base class allows the code to be
RB> reused across platforms). Then, this function simply calls the "Exit()" of
RB> each event loop that we've stored in the hash map.

But if you started discussing the implementation, let me say that this
seems overcomplicated to me. First, we don't need a hash map here, this is
a clear example of a stack: each loop should push itself on the top of the
stack when it's started and pop from it when it's done. Second, AFAICS we
don't even need an explicit stack because we can reuse the implicit stack
of the program. I.e. to implement the new method we just need to unwind the
stack until we reach the loop which is equal to the loop we're trying to
exit.


Anyhow, let me return to the higher level again, implementation
notwithstanding. This ExitAllNestedAndThisLoop() isn't really needed by
your wxExecute() patch, so maybe it wasn't a good idea to start discussing
it. In your patch, you don't add any new functions, but you do change the
semantics of the existing Exit() to mean "exit this loop after all the
nested loops, if any, terminate". And I'm not sure if it's a good idea.
It's one thing to add ExitFromAllLoops() method that explicitly says that
you want to exit from all the currently running loops. It's quite another
to implicitly change Exit() like this. Is it really a good idea outside of
wxExecute() context? And is it actually useful for anything else, in fact?
Maybe the answers to both of these questions are "yes", but why, exactly?


RB> In fact, when you look at the cocoa implementation that I did, you can see
RB> that we actually can't call [NSApp run] recursively, and the solution there
RB> was to re-implement the event loop ourselves by checking/dispatching events
RB> and implementing our own loop deciding when to exit.

Uh oh. It seems dangerous to rely on this code using [NSApp
nextEventMatchingMasl:untilData:isMode:dequeue] having the same effect as
[NSApp run] or CFRunLoopRun(), who knows what important additional things
[NSApp run] might do internally? For instance, I'm almost certain that
we're missing [NSAutoreleasePool drain] call here, because Webkit code does
it manually when using CFRunLoopRun(). But the worst is that we don't know
what else could CFRunLoopRun() do internally. I think we ought to use
CFRunLoopRun() if possible, i.e. when wxEXEC_NODISABLE is not specified, to
make it safer. Also, we can get rid of wxIsMainThread() checks as
wxExecute(SYNC) can only be used from the main thread anyhow.


But, to repeat, the crucial question for me is that of wxEventLoop::Exit()
semantics. Should it really delay-exit the loop even if it's not running by
default? And when would it be useful to do this, other than in wxExecute()
case?

Thanks,
VZ

[Rob Bresalier]

Hi Vadim:

VZ> Uh oh. It seems dangerous to rely on this code using [NSApp
VZ> nextEventMatchingMasl:untilData:isMode:dequeue] having the same effect as
VZ> [NSApp run] or CFRunLoopRun(), who knows what important additional things
VZ> [NSApp run] might do internally? For instance, I'm almost certain that
VZ> we're missing [NSAutoreleasePool drain] call here, because Webkit code does
VZ> it manually when using CFRunLoopRun(). But the worst is that we don't know
VZ> what else could CFRunLoopRun() do internally. I think we ought to use
VZ> CFRunLoopRun() if possible, i.e. when wxEXEC_NODISABLE is not specified, to
VZ> make it safer. Also, we can get rid of wxIsMainThread() checks as
VZ> wxExecute(SYNC) can only be used from the main thread anyhow.

I think this comment is important to discuss first, because it is at the core of providing temporary event loop functionality with wxGuiEventLoop in Cocoa, regardless of the overlapping and wxExecute discussion.  Furthermore the execute patch relies on this (for basic functionality, not just overlapping) because it relies on temporary event loop for sync wxExecute, and the current 2.9.4 temp event loop code does not work.

The way the temp event loop code is currently implemented in 2.9.4 without my patch does not work at all.  If we have a temp loop on top of the main one, with the 2.9.4 implementation as soon as we call Exit() on the temp loop, we will also exit the main loop too (because [NSApp stop] halts both as I have seen in my testing and webkit comments confirm).

So right now, wx does not even have this functionality at all.

So if you want to offer a temporary event loop for general usage, not just for wxExecute, you need to be able to do it without disabling all of the windows - this means NOT using CFRunLoopRun().  So far,  [NSApp

nextEventMatchingMasl:untilData:isMode:dequeue] is the only way I have found to do it.  I have verified that things like timers and GUI events do get handled in this loop.  But I can't say for sure that everything that is required is getting done.

If you look through evtloop.mm, there is already a case where this same [NSApp nextEventMatchingMask:untilData:isMode:dequeue] is used, see wxGUIEventLoop::DoDispatchTimeout().

I have found some sample Apple code where the main loop is overridden (and I think that a redone temp event loop is similar to main loop case), see that example here:

http://developer.apple.com/library/mac/#samplecode/glut/Listings/GLUTApplication_m.html

Also, I found another some other links on the subject:

http://www.cocoabuilder.com/archive/cocoa/305951-nested-runloops.html

http://www.cocoawithlove.com/2009/01/demystifying-nsapplication-by.html

I have also seen in stack traces that the main loop's [NSApp run] calls  [NSApp nextEventMatchingMasl:untilData:isMode:dequeue, which in turn calls CFRunLoopRun().  So I think that CFRunLoopRun() is actually executing indirectly but this code, but I try to catch a stack trace showing that in the coming days.

Also, I have implemented the [NSAutoreleasePool drain] in patch5, see a snippet of the loop code here:

    while ( !m_shouldExit )
    {
        // By putting this inside the loop, we can drain it in each
        // loop iteration.
        wxMacAutoreleasePool autoreleasepool;

...

    }

Inside the ~wxMacAutoreleasePool() destructor, the drain is called, so that will happen on each loop, it is not left out.

VZ> But, to repeat, the crucial question for me is that of wxEventLoop::Exit()
VZ> semantics. Should it really delay-exit the loop even if it's not running by
VZ> default? And when would it be useful to do this, other than in wxExecute()
VZ> case?

I do think this is a generally useful thing to do and not just for wxExecute().

To answer why, we should talk about why one would want to use a temporary event loop at all.

I think the reason that one would want to use it would be to avoid the complexities of implementing asynchronous code.  While async code is usually better, it is harder to implement - you need to save some state for when your async callback occurs, and it is harder to read and understand as you have to read several different functions to understand it.

If you want to have something with the simplicity of synchronous code, but having some of the advantages of async code, using the temporary event loop is a way to do it.  Lets say you have function F, and inside of function F you want to call function S.  And lets say that function S has some delay before it would return, such as it waits for something special to happen that is not a very short amount of time.  You could implement function S in an async way, but it is harder to implement and understand.  

It would easier to program function S as a "sync" type of function.  But while you are waiting for function S to return, you could block with a temporary event loop.  There will be some event that occurs that says function S should return. This event would 'trigger' the exit of the temporary event loop.

So by making Exit() mean " "exit this loop after all the nested loops, if any, terminate", it allows the event that detects the condition that should terminate "S" to tell the event loop associated with S that it can exit.  This logic will not care whether or not the current event loop is the one associated with "S" or a nested one.  It just wants to say that the condition that allows function S to return has been satisfied, and we can return from it.  To make this situation easier to work with, I think we should be allowed to call the Exit() function of any "started" event loop, so that the programmer does not need to keep track of the nested loops.  So I think the assert in Exit() should be removed (or replaced by checking if the loop at least started, as I did in patch5) so that this useful operation can be done.

For my specific case, function "S" is wxExecute(), and the condition that says that "S" can return is the process termination.  But I think this is a general concept that can apply to operations other than wxExecute.

So I think it is something generally useful to allow a simple "synchronous" implementation (using event loop to wait) rather than forcing a programmer to always write a more complex async one.  There are certainly limitations to doing this (such as having to wait for nested event loops), but I think there can be many situations where the limitations are acceptable (especially if the programmer knows for sure that all nested event loops will be short), and I think it can simplify a programmers life.  I guess we could provide an 'out' if really needed, a function to exit all loops as we discussed.

If you don't like "Exit" meaning "exit this loop after all the nested loops, if any, terminate", then maybe we should create another method that does mean this?

Regards,

Rob

[Vadim Zeitlin]

On Wed, 24 Apr 2013 00:36:39 -0400 Rob Bresalier wrote:

RB> VZ> Uh oh. It seems dangerous to rely on this code using [NSApp
RB> VZ> nextEventMatchingMasl:untilData:isMode:dequeue] having the same effect
RB> VZ> as [NSApp run]
...
RB> I think this comment is important to discuss first, because it is at the
RB> core of providing temporary event loop functionality with wxGuiEventLoop in
RB> Cocoa, regardless of the overlapping and wxExecute discussion.

After reading all the links (thanks for providing them!), I agree that
this seems like the right (and only) way to implement overlapping loops in
Cocoa. It's the need for implementing them in the first place that is still
questionable... but I guess we're going to do it, even if it's just to make
wxExecute() working.


RB> Also, I have implemented the [NSAutoreleasePool drain] in patch5
...
RB> Inside the ~wxMacAutoreleasePool() destructor, the drain is called, so
RB> that will happen on each loop, it is not left out.

Sorry for missing it. One other question: what about -[NSApp
updateWindows]? It doesn't seem to be called in the patch, yet it's
documented as being "nvoked automatically in the main event loop after each
event when running in NSDefaultRunLoopMode or NSModalRunLoopMode", see

http://developer.apple.com/library/mac/documentation/Cocoa/Reference/ApplicationKit/Classes/NSApplication_Class/Reference/Reference.html#//apple_ref/doc/uid/20000012-CACCGJID

Shouldn't we call it?


RB> To answer why, we should talk about why one would want to use a temporary
RB> event loop at all.

Notice that the question is more precise than this: I realize the need for
temporary event loops, but I'm less sure about non-modal ones, i.e. those
that

(a) don't disable the UI elements they don't create
(b) can be terminated before the nested loop terminates

Your (again, well-written and perfectly clear) explanation that I snipped
shows that this is necessary for wxExecute() and I definitely agree with
this but I just can't think of any other realistic examples when it would
be needed. Can you?

RB> If you don't like "Exit" meaning "exit this loop after all the nested
RB> loops, if any, terminate", then maybe we should create another method that
RB> does mean this?

Perhaps it would be better, if only because it would give us a place to
explain all this clearly in the documentation. I'm still not sure what
should it be called. ScheduleExit() perhaps?

Regards,
VZ

[Rob Bresalier]

Hi Vadim:

VZ> One other question: what about -[NSApp updateWindows]? It doesn't seem to be called in the patch

I guess it doesn't hurt.  I wish we had more visibility as to the internals of [NSApp run] so we knew exactly what should be in this loop.  I just tried [NSApp updateWindows] locally, my tests still pass.  Also I tried it with my own GUI application, and nothing bad happens.  I notice that the windows do seem to be updated even if updateWindows is not in the loop, but it probably doesn't hurt to still have it.

VZ>  I guess we're going to do it, even if it's just to make wxExecute() working.

I think its the right decision.  Thanks!

VZ> I just can't think of any other realistic examples when it would be needed. Can you?

I can't think of a specific example, its just more of a conceptual idea that I think makes more logical sense.  In fact, I can't really think of why the existing API is particularly useful - to only be able to exit the active event loop.  If someone uses an event loop to simplify asynchronous behavior, I would think that they would only want to schedule an exit of just their associated "blocking" event loop and not need to worry about exiting unrelated inner event loops.  Its a consequence of using event loops for this that gives the potential possibility that you can end up inside another one. I would think that outer loop operation A really does not have any right to cause the exit of inner event loop B.  I think that inner event loop B should only be scheduled for exit when operation B completes.  And it would really complicate the life of the library user to make operation A aware of operation B so that it makes sure to not terminate event loop B.

VZ> RB> If you don't like "Exit" meaning "exit this loop after all the nested
VZ> RB> loops, if any, terminate", then maybe we should create another method that
VZ> RB> does mean this?

VZ>  Perhaps it would be better, if only because it would give us a place to
VZ> explain all this clearly in the documentation. I'm still not sure what
VZ> should it be called. ScheduleExit() perhaps?

I think ScheduleExit() is a good name.  If you agree, I can start coding it up.  Do you think we should have an assert in ScheduleExit() that checks that the event loop was at least started (but not necessarily active) when ScheduleExit() is called?  I had this in my patch - it requires to maintain a flag that indicates the loop was started and not terminated.  What do you think?

Do you think I should also implement this in MSW as well,to have the API available for all platforms?  Or just stick to GTK, Carbon, and Cocoa because that is all that is needed to make wxExecute work?  If we stick to GTK, Carbon and Cocoa, I can reuse my existing test for overlapped wxExecute.  If we do it for MSW, I'll need to write a new test for this, since wxExecute is still using wxYield in MSW.  Its not a big deal to write a new test, I could probably reuse much of the one I already have for overlapped wxExecute.  I know you want to keep the commit small, so I'm guessing you will not want the MSW one now.  On the other hand, this is fresh in our minds and it may make sense to do this now while we have it fresh - because who knows when it could be revisited.  What is your opinion?

Rob

[Vadim Zeitlin]

On Wed, 24 Apr 2013 23:17:57 -0400 Rob Bresalier wrote:

RB> VZ> I just can't think of any other realistic examples when it would be
RB> VZ> needed. Can you?
RB>
RB> I can't think of a specific example, its just more of a conceptual idea
RB> that I think makes more logical sense. In fact, I can't really think of
RB> why the existing API is particularly useful - to only be able to exit the
RB> active event loop.

This is, IMHO, pretty simple to explain: you need to be able to do it to
implement modal dialogs. Such a dialog must be able to terminate the loop
it's running in when a button is pressed.

RB> If someone uses an event loop to simplify asynchronous
RB> behavior, I would think that they would only want to schedule an exit of
RB> just their associated "blocking" event loop and not need to worry about
RB> exiting unrelated inner event loops.

Currently they don't have to because there can be no such inner loops,
everything is always modal. Your patch will change this and this is why I
don't like it very much. But, again, as long as we support wxExecute(SYNC)
at all (which, for the record, I would have never done if I was designing
wxWidgets today), it seems like there is no other solution.

RB> Its a consequence of using event loops for this that gives the
RB> potential possibility that you can end up inside another one. I would
RB> think that outer loop operation A really does not have any right to
RB> cause the exit of inner event loop B. I think that inner event loop B
RB> should only be scheduled for exit when operation B completes. And it
RB> would really complicate the life of the library user to make operation
RB> A aware of operation B so that it makes sure to not terminate event
RB> loop B.

Yes, I can see this point...


RB> I think ScheduleExit() is a good name. If you agree, I can start coding it
RB> up. Do you think we should have an assert in ScheduleExit() that checks
RB> that the event loop was at least started (but not necessarily active) when
RB> ScheduleExit() is called? I had this in my patch - it requires to maintain
RB> a flag that indicates the loop was started and not terminated. What do you
RB> think?

This depends on what happens if the loop isn't started when ScheduleExit()
is called. If, as I suspect, it misbehaves then, i.e. is simply ignored and
the loop would then start and run (forever, presumably) later, then the
assert would be indeed nice to have.

RB> Do you think I should also implement this in MSW as well,to have the API
RB> available for all platforms? Or just stick to GTK, Carbon, and Cocoa
RB> because that is all that is needed to make wxExecute work? If we stick to
RB> GTK, Carbon and Cocoa, I can reuse my existing test for overlapped
RB> wxExecute. If we do it for MSW, I'll need to write a new test for this,
RB> since wxExecute is still using wxYield in MSW. Its not a big deal to write
RB> a new test, I could probably reuse much of the one I already have for
RB> overlapped wxExecute. I know you want to keep the commit small, so I'm
RB> guessing you will not want the MSW one now. On the other hand, this is
RB> fresh in our minds and it may make sense to do this now while we have it
RB> fresh - because who knows when it could be revisited. What is your opinion?

Just to be perfectly clear (and sorry if I'm rehashing the obvious): when
I'm speaking about small commits, I refer to the number of changes in the
commit. If some commit implements a feature and adds documentation and
tests for it and shows it in a sample, it's still a small commit if the
feature itself is "small" and it wouldn't make sense to break it down in
physically smaller but not atomic commits.

So in this case we'd ideally have a patch implementing the new event loop
functionality, i.e. wxEventLoop::ScheduleExit(), for all platforms (with
all the tests, documentation, ...). And having it for MSW would indeed be
good if we consider that it is generally useful and should be public.

And then -- remember I'm speaking about the ideal case -- second, or maybe
second and third, patch(es) with the rest of the changes.

TIA,
VZ

[Rob Bresalier]

Hi Vadim:

I've implemented and tested 95% of the changes we have discussed and have even made changes that I previously commented in the review that I wouldn't have time for (like separating out wxUnixProcess with composition inside of wxProcess, and also replacing the .cpp file that was being used as a .h file in the way you suggested, and some others).  These changes were not that hard to do and test.

I've also implemented the new ScheduleExit() method, and it passes my existing test of overlapping wxExecute test case.

I've reviewed the MSW event loop code and I think it can work overlapped without any changes to it.

However, I need to write a new test to prove that the MSW code works as I think it will.

I can't reuse my existing overlapping test for wxExecute because the MSW wxExecute code still uses wxYield which does not allow overlapping.

I'm struggling as to where to locate this test.

I do not see a folder under the 'tests' subdirectory for event loops.

Do you have any suggestions as to where to place the new event loop test?

Thanks,

Rob

[Rob Bresalier]

Hi Vadim:

Also, would you like the patch to be against the trunk or against 2.9.4 release?

I am working in my own private git branch based off of 2.9.4.  So it should not be hard for me to create another branch that uses the trunk and merges the changes from my current branch off 2.9.4.

I remember few months back when I tried to use the trunk that builds on Ubuntu-wxGTK were failing.  So that is why I reverted to 2.9.4.

But maybe the trunk has been fixed since then?  I feel more comfortable submitting a patch that I am able to test on the 4 platforms I check against - MSW, GTK, Cocoa and Carbon.

Thanks,

Rob

[Vadim Zeitlin]

On Tue, 30 Apr 2013 23:26:03 -0400 Rob Bresalier wrote:

RB> I've implemented and tested 95% of the changes we have discussed and have
RB> even made changes that I previously commented in the review that I wouldn't
RB> have time for (like separating out wxUnixProcess with composition inside of
RB> wxProcess, and also replacing the .cpp file that was being used as a .h
RB> file in the way you suggested, and some others). These changes were not
RB> that hard to do and test.

Great, thank you!

RB> I've also implemented the new ScheduleExit() method, and it passes my
RB> existing test of overlapping wxExecute test case.

Perfect, thanks.

RB> I do not see a folder under the 'tests' subdirectory for event loops.
RB>
RB> Do you have any suggestions as to where to place the new event loop test?

I'd say in tests/events/evtloop.cpp (this would be a new file), why not?

RB> Also, would you like the patch to be against the trunk or against 2.9.4
RB> release?

I'd need to apply it to the trunk, so ideally I'd like to have a patch
against the trunk too but I hope it shouldn't be difficult to rebase it on
the current master anyhow, this part of the code shouldn't have changed
much since 2.9.4.

RB> I am working in my own private git branch based off of 2.9.4. So it should
RB> not be hard for me to create another branch that uses the trunk and merges
RB> the changes from my current branch off 2.9.4.

I'd suggest using rebase (i.e. make a copy of your branch and "git rebase
master" on it) instead of doing the merge.

RB> I remember few months back when I tried to use the trunk that builds on
RB> Ubuntu-wxGTK were failing. So that is why I reverted to 2.9.4.

I test under Ubuntu 12.04 relatively regularly and it builds fine there.
In any case, if there are any compilation problems under Linux, I'd like to
hear about them.

Thanks,
VZ

[Rob Bresalier]

Hi Vadim:

I'm much closer now.  I've got my overlapped event loop test working in MSW and all OSes with the patch applied to 2.9.4 release.

Now, I'm rebased to the trunk and getting a build error on ubuntu.

When I try to build (trunk + my patch) with ubuntu 10.04, I'm getting some build errors.  I did not have these errors before with 2.9.4 release and same configure line.  These errors seem unrelated to my patch (the errors are in tif_ojpeg.c, and I did not modify any 'tif' stuff).

Here is my './configure':

/rob-server/robb/wx/wxgit/wxWidgets/configure --prefix=/home/rob/wx/wxGTK-2.9.4-install-debug --with-gtk=2 --enable-debug --with-libpng=builtin --with-libjpeg=builtin --with-libtiff=builtin --with-regex=builtin --with-zlib=builtin --enable-permissive --with-expat=builtin --enable-unicode --disable-precomp-headers --disable-shared 2>&1 | tee robbmake.log

I get these errors, which seem unrelated to my changes:

/home/rob/wx/wxGTK-2.9.4-build/bk-deps gcc -c -o wxtiff_tif_ojpeg.o -DNDEBUG -I/rob-server/robb/wx/wxgit/wxWidgets/src/zlib -I/rob-server/robb/wx/wxgit/wxWidgets/src/jpeg -I/home/rob/wx/wxGTK-2.9.4-build/src/tiff/libtiff -I/rob-server/robb/wx/wxgit/wxWidgets/src/tiff/libtiff  -D_FILE_OFFSET_BITS=64 -I/home/rob/wx/wxGTK-2.9.4-build/lib/wx/include/gtk2-unicode-static-2.9 -I/rob-server/robb/wx/wxgit/wxWidgets/include -pthread -D_REENTRANT -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/directfb -I/usr/include/libpng12 -pthread -fpermissive -Wall -Wundef -g -O0 -pthread -D_REENTRANT -I/usr/include/gtk-unix-print-2.0 -I/usr/include/gtk-2.0 -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/lib/gtk-2.0/include -I/usr/include/gio-unix-2.0/ -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/directfb -I/usr/include/libpng12 -D_GNU_SOURCE  /rob-server/robb/wx/wxgit/wxWidgets/src/tiff/libtiff/tif_ojpeg.c

cc1: warning: command line option "-fpermissive" is valid for C++/ObjC++ but not for C
/rob-server/robb/wx/wxgit/wxWidgets/src/tiff/libtiff/tif_ojpeg.c:413: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘OJPEGLibjpegJpegSourceMgrFillInputBuffer’
/rob-server/robb/wx/wxgit/wxWidgets/src/tiff/libtiff/tif_ojpeg.c:415: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘OJPEGLibjpegJpegSourceMgrResyncToRestart’
/rob-server/robb/wx/wxgit/wxWidgets/src/tiff/libtiff/tif_ojpeg.c: In function ‘OJPEGWriteHeaderInfo’:
/rob-server/robb/wx/wxgit/wxWidgets/src/tiff/libtiff/tif_ojpeg.c:1164: error: ‘OJPEGLibjpegJpegSourceMgrFillInputBuffer’ undeclared (first use in this function)
/rob-server/robb/wx/wxgit/wxWidgets/src/tiff/libtiff/tif_ojpeg.c:1164: error: (Each undeclared identifier is reported only once
/rob-server/robb/wx/wxgit/wxWidgets/src/tiff/libtiff/tif_ojpeg.c:1164: error: for each function it appears in.)
/rob-server/robb/wx/wxgit/wxWidgets/src/tiff/libtiff/tif_ojpeg.c:1166: error: ‘OJPEGLibjpegJpegSourceMgrResyncToRestart’ undeclared (first use in this function)
/rob-server/robb/wx/wxgit/wxWidgets/src/tiff/libtiff/tif_ojpeg.c: At top level:
/rob-server/robb/wx/wxgit/wxWidgets/src/tiff/libtiff/tif_ojpeg.c:2446: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘OJPEGLibjpegJpegSourceMgrFillInputBuffer’
/rob-server/robb/wx/wxgit/wxWidgets/src/tiff/libtiff/tif_ojpeg.c:2472: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘OJPEGLibjpegJpegSourceMgrResyncToRestart’
make: *** [wxtiff_tif_ojpeg.o] Error 1

Any ideas?

Lines 413 and 415 are:

413 static boolean OJPEGLibjpegJpegSourceMgrFillInputBuffer(jpeg_decompress_struct* cinfo);
415 static boolean OJPEGLibjpegJpegSourceMgrResyncToRestart(jpeg_decompress_struct* cinfo, int desired);

I think maybe it does not like 'boolean' type?

Thanks,

Rob

[Vadim Zeitlin]

On Tue, 7 May 2013 00:02:34 -0400 Rob Bresalier wrote:

RB> I'm much closer now. I've got my overlapped event loop test working in MSW
RB> and all OSes with the patch applied to 2.9.4 release.

Great!

RB> When I try to build (trunk + my patch) with ubuntu 10.04, I'm getting some
RB> build errors. I did not have these errors before with 2.9.4 release and
RB> same configure line. These errors seem unrelated to my patch (the errors
RB> are in tif_ojpeg.c, and I did not modify any 'tif' stuff).

As a quick workaround, you can always use --without-libtiff. But it should
work with it too, of course.

RB> I think maybe it does not like 'boolean' type?

It looks like this, indeed. Does it manage to compile tif_jpeg.c file
(you can run "make wxtiff_tif_ojpeg.o" to check for it)? AFAICS normally
boolean should be defined in jmorecfg.h, so either something predefines
HAVE_BOOLEAN or I'm missing something... To check for HAVE_BOOLEAN, could
you please insert

#define HAVE_BOOLEAN something_unique_here

line before jpeglib.h inclusion in tif_ojpeg.c? This should result in a
compiler warning about macro redefinition pointing out to where it was
previously defined if it's indeed the case.

Thanks,
VZ

[Rob Bresalier]

Hi Vadim:

VZ> Does it manage to compile tif_jpeg.c file

Yes, but I notice there is no "boolean" in there.

VZ> To check for HAVE_BOOLEAN, could
VZ> you please insert
VZ> 
VZ>        #define HAVE_BOOLEAN something_unique_here
VZ>
VZ> line before jpeglib.h inclusion in tif_ojpeg.c? This should result in a
VZ> compiler warning about macro redefinition pointing out to where it was
VZ> previously defined if it's indeed the case.

When I insert this, there is no compiler warning at all.  Looks like "HAVE_BOOLEAN" is not defined.

Looking through the code, it looks like the purpose of 'wxjpeg_boolean' is to replace 'boolean' type, as 'boolean' may be defined by some compilers.  So I did a search/replace inside of tif_ojpeg.c to replace 'boolean' with 'wxjpeg_boolean', and now it compiles!  Not only that, but all tests on Ubuntu 10.04 pass both for console and GUI tests!

When I search through all of the wx library code, it looks like straight 'boolean' is never used anywhere, and the purpose of wxjpeg_boolean was to replace it.

I'll include this in my patch just so you can see it.

Rob

[Vadim Zeitlin]

On Tue, 7 May 2013 23:50:42 -0400 Rob Bresalier wrote:

RB> Looking through the code, it looks like the purpose of 'wxjpeg_boolean' is
RB> to replace 'boolean' type, as 'boolean' may be defined by some compilers.
RB> So I did a search/replace inside of tif_ojpeg.c to replace 'boolean' with
RB> 'wxjpeg_boolean', and now it compiles! Not only that, but all tests on
RB> Ubuntu 10.04 pass both for console and GUI tests!
RB>
RB> When I search through all of the wx library code, it looks like straight
RB> 'boolean' is never used anywhere, and the purpose of wxjpeg_boolean was to
RB> replace it.

Yes, you're right, I've realized it myself later as well, see my comment at
http://trac.wxwidgets.org/ticket/15179#comment:2, I should have followed up
here. I'll apply your fix, thanks!

And I'll try to deal with the latest version of the patch a.s.a.p. too,
VZ