wxExecute() blues

[Vadim Zeitlin]

Hello,

I'm posting here for two reasons: first, to expand on the comments I left
at http://review.bakefile.org/r/471/ for Rob's benefit. And second, to
illustrate the kind of problems that I'm wary of when speaking about
applying big and non-trivial patches.

So, I finally convinced myself to spend whatever time it takes to finish
reviewing and merging this patch today (the recent discussion about the
people being frustrated by their patches taking a long time to be accepted
added some extra motivation). As a reminder for the others, the patch fixes
the process termination handling in wxExecute() under Unix and also allows
nested calls to wxExecute(). This is not the most critical feature of wx
but it's definitely important enough and I had encouraged Rob to make this
patch and he spent a lot of time working on this so, in addition to the
benefits to wxWidgets, I also felt rather obliged to finish integrating it
to ensure that Rob's efforts were not for nothing.

Unfortunately after spending 5 hours on this (which is about how much
I typically spend on wx in a week), I'm still no closer to a satisfactory
version. The trouble is that I've belatedly realized that we have two
completely different mechanisms for monitoring file descriptors for
activity. One is wxEventLoop::AddSourceForFD() method which is relatively
generic and another one is specific to wxExecute() and is drastically
extended (and made more complex) by this patch.

And I don't like this because I'd really like to make AddSourceForFD() an
official, publicly documented method one of these days. And having another
parallel API with overlapping functionality but completely different
implementation won't help with this at all.

Unfortunately using AddSourceForFD() for wxExecute() is not easy neither,
especially on Mac side where the latter uses sockets for whatever reason
instead of plain file descriptors. It could be safe to stop using sockets.
Or not. I really don't know. In any case, replacing all the current
wxFDIOManager-based code with wxEventLoopSourceHandler-based version will
take at least several more hours.

So I can:

1. Commit the current patch with some cleanup (partially already done,
it would be nice to do a bit more of it, e.g. get rid of all these
global hash maps and have just one of them in common code, but this is
less critical) and leave with even worse mess than what we have now.

2. Spend 5 or 10 or more hours on this to really do it as I think it
should be done, even if this clearly means not working on other,
equally or even more important, things in wxWidgets and hence postponing
2.9.5 and 3.0 releases even more. And taking into account the time
required, "more" may mean a couple of weeks -- in addition to the time
that fixing all the other 2.9.5-critical problems is going to take.

3. Drop all these changes (except for the changes only affecting
wxEventLoop which have been already fully reviewed and merged), in
spite of both Rob and me spending so much time on this already.

I really hate all these possibilities but I have to choose one of them as
not choosing basically means choosing (3) which is arguably the worst of
all. I guess I'm going to try to do (2), after all, but it will probably
take me quite some time to just gather enough courage to do it.

And the saddest thing is that I still don't know how to avoid such
problems in the future. I'd like to learn something from this debacle but I
really don't know what... I guess this is another reason -- in addition to
quite a bit of frustration I'm feeling right now -- I'm posting this here.
Perhaps something could have been done differently to avoid finding oneself
in such situation? If so, what and how? Again, I do see at least two
possibilities but none of them is really appealing: the first one is to
accept all patches (or at least all those satisfying the formal
requirements of having the documentation and passing the tests) regardless
of their contents -- but I can't convince myself that this is not going to
result in a complete disaster pretty soon. The second one is to only accept
patches which don't require any further work -- but this will mean throwing
away valuable contributions too. I've been trying a third one: to review
the patches but ask the people to change things that don't look right
instead of doing it myself, but this clearly doesn't work in practice. Rob
has done a lot of efforts to address my requests but there is still a ton
of changes between his version at http://review.bakefile.org/r/471/ and
mine at https://github.com/vadz/wxWidgets/commits/unix-execute, and I
didn't even realize the really fundamental problem by just looking at the
code anyhow.

Is there a better way?

Thanks for reading,
VZ

[Eric Jensen]

Hello Vadim,

Sunday, June 9, 2013, 10:09:13 PM, you wrote:

VU> the first one is to
VZ> accept all patches (or at least all those satisfying the formal
VZ> requirements of having the documentation and passing the tests) regardless
VZ> of their contents
i would agree that this is not an option. As a user of wxWidget i like
to be able to trust the library - it's more important to have
stable, tested code than having new untested and potentially
unreliable features.


VZ> I've been trying a third one: to review
VZ> the patches but ask the people to change things that don't look right
VZ> instead of doing it myself, but this clearly doesn't work in practice.
I guess this is the way to go, although you will still have to decide
on a case-to-case basis.

I haven't submitted any big patches myself, but even the relatively
small one regarding WM_PAINT handling and re-using of paintdc pretty
much demonstrated the same problems you were talking about.

First i need to understand the wx code. Unfortunately, especially
under MSW, there is often code to work around Windows oddities,
which makes it harder to understand. And if i don't understand the
code 100%, it's even harder to add my changes and still making sure
not to break anything in the process.

Judging from the differences between my code and the final code, i
estimate that you still had to spend at least 2-3 hours on it before
you applied it to wx.

I think it can be expected from potential patch submitters to make
changes to their code on demand. Even if not to a stage where it
can applied immediately, but at least to a stage where it's relatively
close to that. If they just want to fire-and-forget a piece of code at
Trac, they must be prepared that eventually their code might not be applied.

Even if this may stop some people from submitting, for efficiency
reasons there is no other way.

I'd like to add that missing new features are hardly any show-blockers. Two
of my main wx projects that i maintain are still based on wx 2.6.1 and
i'm be totally happy with that. Many improvements in later wx
versions are "nice to have", but not really necessary for most
applications. Like i said above, tested and reliable code is more
important than new features (at least to me).

Unfortunately i don't have any magical solution that will make
applying new patches easy for all sides - but i also think there is
none.

Regards,
Eric

[Manolo]

Hello, I want to put my two cents on this.

IMHO the main problem for a patch to be applied is not only if it works
or not. It's the way it's coded.

All of us knows that different people do same things in different ways.
And it's also common that a person doesn't like the way of other person,
regardless of if this other way may work or not. As you suppose, I'm
speaking of Vadim. He's the main maintainer, the one who stays despite
of whatever it happens, day after day. I admire him. But the dark side
of this story is that he knows very very well that sooner or later he
will meet some problem from another guy's code. And trying to repair
a code you're not used to, becomes hard (try it and you'll see why).

Therefore, wxWidgets needs more people like Vadim. Let me reword it:
more people that code like Vadim does, or close to. Patchers may be
trained in wx-style if they want their code to be applied.

Of course, Vadim could close his eyes, just test the patch to work,
and apply it. I don't think of this to be the solution. But this also
shows that a predefined test-protocol may avoid most of future problems,
so Vadim would half-close his eyes.

What test-protocol? This is the tricky part. Wx currently uses CPPUNIT
as the test tool. I believe that extending this protocol would do [most
of] the job. For example, each control:
a) Should have predefined auto-tests, like CPPUNIT ones.
b) Should have a sample that shows _all_ of its features.
c) There should be a questionnaire for patchers/testers, with questions
such as "Does feature 'x' work?" "Does feature 'y' work when action 'z'
takes place?"
d) Notice that this questionnaire may just repeat the list of the
requirements for the control.
Any patch for a control should fulfill the protocol, extending the
tests, sample, and docs.

While a) and b) currently exist, that's not the case for c) and d).

Notice that d) should be also part of the documentation, so any user can
know what that control is able of.

Despite of these ideas, I'm afraid that a common patcher want put so
much dedication on his patch. Things will go as they go now. And then we
arrive again to the beginning: patches done in wx-style, Vadim's way.

Regards,
Manolo

[Armel]

Hi,

 

Le dimanche 9 juin 2013 22:09:13 UTC+2, VZ a écrit :

Hello,

... 

I believe that one of the main problem is the lack of publicity of target specifications and design choices for a particular release.

In your mail, you write about the AddSourceForFD() which should have been the principle to improve existing wxExecute code, how could Rob know at first? (it is just a factual question, is it written somewhere? I could not find anything neither on the website nor in the code)

 

What I could find when searching is merely a list of 10 to 100 words long subjects in a TODO list (that is: http://wiki.wxwidgets.org/Development:_Todo_List), the document about wxTNG (http://wiki.wxwidgets.org/Development:_wxTNG) was not modified for 3 years (last update may 2010). In fact I realise when reading again that the TODO list is in the same state, is it abandonned as well?

 

The reference establishing "what to do" for a particular subject cannot either be just a sequence of emails or a feature request list. While these methods can work for small projects with a fast pace, a long term project a la WX cannot suffer that.

 

My own contributor experience was typically quite good, except for the search highlighting in HTML; that is the only 'big' feature that I proposed, I just could not catch what was wrong finally with what I proposed, communication is not always simple by mail. The patch is still living well in my own tree BTW.

From that angle, the point is probably that wxWidgets team does not tighten enough what is wanted as contributions for a release x on certain subjects (so that there is no deep will to make the patches pass but time is passed on them). I could have lived perfectly with my patch being temporarily rejected (as it is in fact), and the WX team coming back months or years after my proposal just asking me to publish because there is a need for it or real will, and thus time, to cope with my imperfections.

 

If I was in charge of WX, my first action would be to tell what I expect from contributors, not just in term of mechanics of contribution but really what is in your wx-team mind of the exact target of next stable release. The perimeter of releases should be tight enough to fit a schedule.

Did I missed good documentation establishing all of that?

 

You all wx team (and contributors), your work is excellent so we need to continue this great work. We just need a bit more of anticipation in the work coordination.

Thanks for your work,

Armel

[Rob Bresalier]

Hi Vadim:

Thank you very much for your time spent on this patch.  I pulled down your branch into my local git repository, and I was able to build it and it passed tests on GTK, Cocoa, and Carbon!

I honestly did not know about the other API, wxEventLoop::AddSourceForFD().  As you know, I had seen there was already an existing API for handling the child termination fd (before the patch), and expanded that.  I wish that I looked around more.  I do remember seeing some similar code in the wx sockets implementation, but it looked too specialized for sockets to be reused in wxExecute.  I think you also want to replace that code in the sockets implementation with the wxEventLoop::AddSourceForFD() too (for another day!).

I think sometimes issues like this are unavoidable, and in real life nothing is perfect and we can just learn from mistakes or oversights and continually correct, iterate and make improvements.

This is not a small patch.  Also, you are very busy.  It takes time to understand all of this patch.  As you are very busy, it took you a while just to get enough time to spend on it to fully understand it.  Then, even while you were working on it for a few hours, you also realized belatedly that there was another existing API for setting up and removing fd callbacks.  You have many, many patches going on, wxWidgets library is very large, so I can understand why it is difficult to remember everything that is already in the code.

I think there is still much in the wxExecute patch outside of the fd callback setup/removal that does not change with a transition to wxEventLoop::AddSourceForFD(), so there is much already here that does not need to be rewritten, and we should not throw away the valuable time that both you and I spent on it.

I took a look at the wxEventLoop::AddSourceForFD() API, and I see it is more general than what I did for wxExecute() (handles write and exception, in addition to read events).  I agree that it is the right path to pursue.

I am willing to spend the time to modify the code (starting with the code in your unix-execute branch - and I admire your refactoring of it) to transition to this API.  If you are willing to have me spend time on it, then you can tell me how you would proceed if you were doing this patch yourself and I can start on that path.  Then you will be freed up to work on other wx issues until I have it working and debugged and it is ready for you to look at it again.

My main fear here however is about the stability of wxEventLoop::AddSourceForFD().  It is clearly very useful and something that I think would be a great addition to the library as a public API.  So I want to know why it is not a public API already?  I'm willing to see how well the API works when used with wxExecute(SYNC), so I may be debugging code here and I'd like to know what you know about stability and maturity?

Another question I have is why is this API tied to wxEventLoop class?  The reason I ask is related to our recent addition of nested/overlapped event loops.  It seems to me that this API should only ever be called using the main loop's event loop instance and never with a nested instance of wxEventLoop.  So how to prevent calling it from a nested event loop?  Wouldn't the application need to first get an instance of the main event loop and then use that to call the API?  So maybe an API in the application class would be favored by the library user, as they would not need to worry about getting the main loop object.  This API in the wxApp class would then get a pointer/reference to the main loop object internally and then call the main loop's AddSourceForFD().  Maybe the public API would be the one in the wxApp class, but we would not throw away everything the wxEventLoop, we would just shield it from the public and have the public API in wxApp call the wxEventLoop::AddSourceForFD()?

Regards,

Rob

[Vadim Zeitlin]

On Tue, 11 Jun 2013 00:40:09 -0400 Rob Bresalier wrote:

RB> Thank you very much for your time spent on this patch.

Hi,

First of all, thanks for taking this so well, I really appreciate the fact
that you discuss the technical merits of this approach even though it must
be frustrating for you to have done at least part of the work you did in
vain.

RB> I am willing to spend the time to modify the code (starting with the code
RB> in your unix-execute branch - and I admire your refactoring of it) to
RB> transition to this API. If you are willing to have me spend time on it,
RB> then you can tell me how you would proceed if you were doing this patch
RB> yourself and I can start on that path.

I would, of course, be thrilled if you could reimplement wxExecute code to
use AddSourceForFD() API. The remaining refactoring concerns "just" the
last commit on the 10258-sync-execute branch (the one with "WIP" in its
subject -- and I put "just" in the quotes because it contains about 80% of
the original patch). I did quite a few changes to this commit too, notably
to the tests code, which I think is good to go, but not to the core of the
changes.

As for how exactly to proceed, I can't be really 100% sure about this but
the guiding principle is always the same: try to do as small as possible
atomic changes, so that each of them is simple to review and test. In
particular, prefer to separate refactoring (which shouldn't result in any
changes in behaviour) from the new features addition.

In this case, I think the first step could be to change the existing
AddProcessCallback() to use AddSourceForFD(). And then change the process
termination detection code to use SIGCHLD.

Practically speaking, you should branch from 10258-sync-execute~1 commit,
and then apply new changes on top of it. I don't go into details of how to
do it because I assume that you're familiar with Git but if you're not,
please let me know and I'll explain it more.


RB> My main fear here however is about the stability of
RB> wxEventLoop::AddSourceForFD(). It is clearly very useful and something
RB> that I think would be a great addition to the library as a public API.
RB> So I want to know why it is not a public API already?

The main reason is that there is no equivalent under Windows. We ought to
have AddSourceForHandle() there to allow monitoring arbitrary HANDLEs in
the main loop there, but this hasn't been done yet. It's not a requirement
to have a feature implemented on all platforms before it's made public, of
course, but it helps in case we need to change the API later to make it
possible to implement it elsewhere. E.g. the final API I had in mind would
be to have just wxEventLoop::AddSource(wxEventLoopSource) and then the user
code would create its event loop source in whatever way it needs it to be
done (from FD, from HANDLE, from something else). But we could always keep
AddSourceForFD() as a convenient helper, of course.

And, also, I think in this case the functionality is so useful that we
should probably make it available even if it's Unix-only. And, after all,
it's much more common for Unix programs to monitor some other file
descriptors, than for Windows programs to do the same with some HANDLEs.

So basically I think we should make it public and I hope to do it before
3.0.

RB> I'm willing to see how well the API works when used with
RB> wxExecute(SYNC), so I may be debugging code here and I'd like to know
RB> what you know about stability and maturity?

A couple of my own programs do use it and I haven't seen any problems with
it, so I'd say it's stable in the terms of functionality. The API could
change though, notably if you find any problems preventing it from being
used with wxExecute() right now.


RB> Another question I have is why is this API tied to wxEventLoop class? The
RB> reason I ask is related to our recent addition of nested/overlapped event
RB> loops.

Well, yes, it's related to this and this discussion has made me change my
mind about some things so I don't know if I would have still done it like
this if I was adding AddSource() now. But then some things concerning the
nested event loop behaviour are actually still not very clear to me...

RB> So how to prevent calling it from a nested event loop?

I am not sure it should be prevented. It is, in principle, quite possible
to need to monitor an FD only while a nested event loop is running. And, in
fact, that could well be the case for sync wxExecute() (although I don't
say it has to be done like this).

A more important question for me is whether a nested event loop should be
always monitoring the sources monitored by its outer event loop. I think it
should, because you don't want your background activity to stop just
because a modal dialog is shown. But OTOH this does raise quite a few
additional questions, such as what exactly happens if RemoveSource() is
called on various objects?

Another alternative would be to have a single set of event loop sources,
reused by all loops. This looks simpler to me right now but I'm not sure if
it really has no drawbacks.

RB> Wouldn't the application need to first get an instance of the main event
RB> loop and then use that to call the API? So maybe an API in the application
RB> class would be favored by the library user, as they would not need to worry
RB> about getting the main loop object.

If we go with the "single global event loop set" approach, then I'd
prefer to make AddSource() a static method of wxEventLoop rather than put
it in wxApp. It seems logical to me to have this in wxEventLoop as the
sources are only monitored while an event loop is running. And I also
prefer to avoid adding yet more methods to wxApp because it already has
quite a lot (and IMO too many) of them.

Still, the main outstanding question is whether each event loop should
have its own set of sources or if the sources should be global and used by
all event loops. If anybody has thoughts about this, they would be very
welcome.

TIA,
VZ

[Vadim Zeitlin]

Hello,

First of all, thanks to Armel, Eric and Manolo for your replies, I
appreciate having your different point of views (although the impossibility
to find a magic solution seems to be a common point :-).


On Sun, 9 Jun 2013 23:16:10 +0200 Eric Jensen wrote:

EJ> Judging from the differences between my code and the final code, i
EJ> estimate that you still had to spend at least 2-3 hours on it before
EJ> you applied it to wx.

FWIW I'm pretty sure you overestimate it in this case. I did change some
things but mostly this was just trivial refactoring IIRC. Your initial
patch was fundamentally sound and I didn't do any really big changes. But
then it was a relatively small patch (albeit an important one).

EJ> I think it can be expected from potential patch submitters to make
EJ> changes to their code on demand. Even if not to a stage where it
EJ> can applied immediately, but at least to a stage where it's relatively
EJ> close to that. If they just want to fire-and-forget a piece of code at
EJ> Trac, they must be prepared that eventually their code might not be applied.

I'm afraid I have to agree with this. I simply won't ever have time to be
able to do it myself. It may be difficult to justify this to people who did
make an effort to submit a patch but all we can do is to try to be more
pedagogical about it, probably.

EJ> I'd like to add that missing new features are hardly any show-blockers.

In principle, I agree, but in practice any sufficiently big patches bit
rot relatively quickly, i.e. it's pretty rare to be able to apply them a
year after they were made, even manually -- there are just too many changes
happening to all parts of wx code. So not applying them now may mean not
doing it ever (or at least until someone else comes and redoes them). Which
could still be the right thing to do, but it's hard to ignore the appeal of
a new shiny feature sometimes.

But, just for the record, wxExecute() patch is mostly a bug fix and not a
new feature, even though it does allow to do some things which were
impossible before as a kind of side effect.


On Mon, 10 Jun 2013 02:35:38 +0200 Manolo wrote:

M> IMHO the main problem for a patch to be applied is not only if it works
M> or not. It's the way it's coded.

I don't really agree with this. The trivial problems such as indentation,
spaces and variable naming are not really show stoppers and can be dealt
with easily and quickly. They do annoy me because I still have to do it and
I often have a feeling that if a person couldn't be bothered to indent his
code properly, it's unlikely that the other things were done well neither.
But in this sense "the way it's coded" is more a symptom of other problems
rather than a problem on its own.

M> But the dark side of this story is that he knows very very well that
M> sooner or later he will meet some problem from another guy's code. And
M> trying to repair a code you're not used to, becomes hard (try it and
M> you'll see why).

Yes, exactly, you've really put a finger on it here.


M> Therefore, wxWidgets needs more people like Vadim. Let me reword it:
M> more people that code like Vadim does, or close to. Patchers may be
M> trained in wx-style if they want their code to be applied.

I don't think it's necessary to code as I do (I don't even know if I have
any specific personal style, just the general principles which are, I
think, pretty well accepted in the wider community, e.g. things like "don't
repeat yourself" and "opened/closed principle").

What we really need are people who would be co-maintaining (parts or
whole of) wxWidgets all the time. So, once again, if anybody would like to
become a maintainer of some of the subsystems, you're very welcome to
apply.

Just as an example, when Paul Cornett became wxGTK maintainer, this really
helped a lot as I didn't have to deal with tons of wxGTK bugs and patches
that he took care of (this is in addition to doing most of the work of
porting wxGTK to GTK+3 which I would have never had time for anyhow).

And right now we basically have Paul, Stefan, who maintains wxOSX to the
best of his availability, Julian who works on wxRTC and Steven who takes
care of wxWebView. But, and even though many other people contribute more
or less permanently to wx, all the other areas, including the notorious
wxAUI, wxDVC and wxGrid components, don't have any dedicated maintainers,
i.e. somebody who would be always available to at least give an advice
about dealing with the problem, if not solving it. And taking important
decisions, such as the one recently discussed in wxAUI thread.


M> Any patch for a control should fulfill the protocol, extending the
M> tests, sample, and docs.

This is already de facto the case. The samples part is still somewhat
optional but I don't apply any patches adding new features without the
tests and the docs any more. It probably could be improved, i.e. adding
extra questions to be answered when submitting a patch might be a good idea
but I'm not really sure what could these questions be.


On Mon, 10 Jun 2013 02:36:14 -0700 (PDT) Armel wrote:

A> I believe that one of the main problem is the lack of publicity of target
A> specifications and design choices for a particular release.

This is definitely a problem (and my own fault, too). But I don't think
it's really related to the question about how to deal with the
not-quite-ideal patches and avoid spending too much time on them
accidentally, which was the point of my original email. Unless you think we
should stop accepting any patches at all and just make the release. Which
is actually something I've been seriously thinking about. But OTOH it's
hard to decide to ignore a patch fixing a real problem and knowingly
let it be present in a new major release.

A> In your mail, you write about the AddSourceForFD() which should have been
A> the principle to improve existing wxExecute code, how could Rob know at
A> first?

He couldn't, of course.

A> What I could find when searching is merely a list of 10 to 100 words long
A> subjects in a TODO list (that is:
A> http://wiki.wxwidgets.org/Development:_Todo_List), the document about wxTNG
A> (http://wiki.wxwidgets.org/Development:_wxTNG) was not modified for 3 years
A> (last update may 2010). In fact I realise when reading again that the TODO
A> list is in the same state, is it abandonned as well?

I've never touched these pages on that wiki, I try to maintain only the
"official" http://trac.wxwidgets.org/wiki/Roadmap page. And even with it I
fail more often than not.

A> My own contributor experience was typically quite good, except for the
A> search highlighting in HTML; that is the only 'big' feature that I
A> proposed, I just could not catch what was wrong finally with what I
A> proposed, communication is not always simple by mail. The patch is still
A> living well in my own tree BTW.

This is an example of the same problem I mentioned above: lack of
maintainer for this particular area. Vaclav does take care of some wxHTML
patches still but he doesn't always have time for them...

A> From that angle, the point is probably that wxWidgets team does not tighten
A> enough what is wanted as contributions for a release x on certain subjects

I think we do, but this is not how the contributions happen. I don't think
we're going to find much success by loudly proclaiming that we need this
and that. Instead, people work on things they are affected by, i.e. scratch
their own itch. And I think it's quite all right, this is how all the
original open source projects worked and I remain convinced that it's a
viable model. We just need to learn to deal better with it than we do now.
If possible.

A> If I was in charge of WX, my first action would be to tell what I expect
A> from contributors, not just in term of mechanics of contribution but really
A> what is in your wx-team mind of the exact target of next stable release.
A> The perimeter of releases should be tight enough to fit a schedule.
A> Did I missed good documentation establishing all of that?

The roadmap I linked to above is the closest thing we have to this.

A> You all wx team (and contributors), your work is excellent so we need to
A> continue this great work. We just need a bit more of anticipation in the
A> work coordination.

Yes, I can't disagree with this. We need to do something concrete about it
though, and I'm not sure what exactly. Any suggestions -- or contributions
-- would be very welcome, as always.


Thanks again for your replies (and for all those who read my original long
message even without replying to it :-)!
VZ

[Rob Bresalier]

Hi Vadim:

VZ> First of all, thanks for taking this so well, I really appreciate the fact
VZ> that you discuss the technical merits of this approach even though it must  
VZ> be frustrating for you to have done at least part of the work you did in 
VZ> vain. 

I don't consider it to be in vain.  I learned quite a bit about file descriptor handling in Unix.  It was a great learning experience.  I think that even with this new refactoring, much of the work that I did remains (particularly the handlers/OnReadWaiting() and code called from there).
 
VZ> Practically speaking, you should branch from 10258-sync-execute~1 commit, 
VZ> and then apply new changes on top of it. I don't go into details of how to 
VZ> do it because I assume that you're familiar with Git but if you're not, 
VZ> please let me know and I'll explain it more. 

Agreed, I already started from your final commit (the one you called 'WIP') and branched from there.  Just for clarification, your branch is named 'unix-execute' and not '1025-sync-execute~1', correct?

VZ> Another alternative would be to have a single set of event loop sources, 
VZ> reused by all loops. This looks simpler to me right now but I'm not sure if 
VZ> it really has no drawbacks. 

I'm very much in favor of this approach.  I've been using the nested event loops in my application and it is really useful for coding asynchronous behavior in a synchronous way.  So I think all nested event loops would want to handle all the same events.
 
VZ> Still, the main outstanding question is whether each event loop should 
VZ> have its own set of sources or if the sources should be global and used by 
VZ> all event loops. If anybody has thoughts about this, they would be very 
VZ> welcome. 

My thoughts are that the sources should be global to all event loops (I did not even think about the other alternative until you brought it up).   If the library user wants to disable events during a nested event loop, then they can code that into their application (disabling/enabling the event before/after calling wxEventLoop::Run()).

VZ> I think the first step could be to change the existing AddProcessCallback() to use AddSourceForFD().

I started coding this and I think that I actually need 2 versions of AddProcessCallback() - one for GUI applications (which calls wxTheApp->GetMainLoop()->AddSourceForFD) and a second for console applications (which uses wxFDIODispatcher::Get()->RegisterFD).  For console applications, there is no main event loop.  But the library user can still call wxExecute(ASYNC) and create the event loop themselves after calling wxExecute(ASYNC).  This means that I have no reference/pointer to the event loop that I can use to call AddSourceForFD().  So I need to use wxFDIODispatcher::Get()->RegisterFD() instead in order to register the fd before I return from wxExecute(ASYNC).

The 2 different methods for GUI vs. Console leads to some more complications.  AddSourceForFD() wants a wxEventLoopSourceHandler* as a parameter while RegisterFD() wants a wxFDIOHandler* as a parameter.  This forces me to use multiple inheritance on both wxWakeUpPipe and wxExecuteIOHandler, to make each be derived from both wxFDIOHandler and wxEventLoopSourceHandler.

Then, because these handlers are derived from 2 classes, I can't just use a wxFDIOHandler* as the parameter to AddProcessCallback() anymore, as wxEventLoopSourceHandler* must be used instead for GUI applications.  So I need 2 different functions now - I will convert AddProcessCallback() to take wxExecuteIOHandler* as a parameter.  For the wake up pipe, I created new function called AddWakeupCallback() which takes wxWakeUpPipe as a parameter.  Both of these functions will have console and GUI versions and are almost identical except for the type of parameter they take.  I guess we could reduce from 4 functions to 2 functions if we make a common base class that is derived from both wxFDIOHandler* and wxEventLoopSourceHandler*, but I'm not sure the duplication is that bad so maybe a new base class is not worth it?

I am also running into a new complication.  With the current state of the wxWidgets GUI code, I don't see a way to stop the callbacks on the fd without deleting the event source object (I can't find a RemoveSourceForFd() function).  I'm not sure that deleting the object will work here because I think this object is still used and needed after the child process is finished.  I also remember that if I don't disable the callbacks after we get EOF on stdout/stderr (wxExecute(SYNC)) that they keep getting called repeatedly, deadlocking the application.  Therefore, I think I will be required to add the RemoveSourceForFd() function.  If you agree, please give me your guidance on how you would like me to do that (function names, which class to put it in, etc).

VZ> And then change the process termination detection code to use SIGCHLD. 

I see that this is already done in your last commit, you took this from my patch.  So am I missing something, or did you just forget that it is already done?

Regards,
Rob

[Vadim Zeitlin]

On Tue, 11 Jun 2013 21:23:38 -0700 (PDT) Rob Bresalier wrote:

RB> Agreed, I already started from your final commit (the one you called
RB> 'WIP') and branched from there. Just for clarification, your branch is
RB> named 'unix-execute' and not '1025-sync-execute~1', correct?

Yes, sorry for the confusion. It's called 10258-sync-execute locally but I
pushed it as unix-execute for some reason... As for "~1", it was just meant
to indicate the last before one commit in it, in Git notation.

RB> VZ> Another alternative would be to have a single set of event loop sources,
RB> VZ> reused by all loops. This looks simpler to me right now but I'm not sure if
RB> VZ> it really has no drawbacks.
RB>
RB> I'm very much in favor of this approach. I've been using the nested
RB> event loops in my application and it is really useful for coding
RB> asynchronous behavior in a synchronous way. So I think all nested
RB> event loops would want to handle all the same events.

OK, let's start with this. I still have a feeling that this is going to
create problems in some situations but I don't have any precise arguments
against it so this is not really convincing.

I also do agree that this is probably what you want in the common case.
And I guess we could extend this API by providing some AddLocalSource()
registering the given source with this event loop only in the future if we
really need it.


RB> VZ> I think the first step could be to change the existing
RB> VZ> AddProcessCallback() to use AddSourceForFD().
RB>
RB> I started coding this and I think that I actually need 2 versions of
RB> AddProcessCallback() - one for GUI applications (which calls
RB> wxTheApp->GetMainLoop()->AddSourceForFD) and a second for console
RB> applications (which uses wxFDIODispatcher::Get()->RegisterFD).

For me these two different versions are two different implementations of
the same virtual function in wxConsoleEventLoop and wxGUIEventLoop
respectively.

RB> For console applications, there is no main event loop.

Notice that it's perfectly possible to have an event loop in console
programs too. But it's equally possible to not have it.

RB> But the library user can still call wxExecute(ASYNC) and create the
RB> event loop themselves after calling wxExecute(ASYNC).

Let's just disallow this. It's not a huge thing to ask to create the event
loop before calling wxExecute(ASYNC), it's not like you're going to be able
to avoid doing it if you're using anything async.

I.e. just insert a wxCHECK_MSG() in wxExecute() verifying that an event
loop is available.

RB> The 2 different methods for GUI vs. Console leads to some more
RB> complications. AddSourceForFD() wants a wxEventLoopSourceHandler* as a
RB> parameter while RegisterFD() wants a wxFDIOHandler* as a parameter.

Again, this is one of the things we're going to get rid of by always using
AddSourceForFD().

RB> This forces me to use multiple inheritance on both wxWakeUpPipe and
RB> wxExecuteIOHandler, to make each be derived from both wxFDIOHandler and
RB> wxEventLoopSourceHandler.

Both of them should derive only from wxEventLoopSourceHandler.

RB> Then, because these handlers are derived from 2 classes,

We definitely must avoid this in any case.

RB> I am also running into a new complication. With the current state of
RB> the wxWidgets GUI code, I don't see a way to stop the callbacks on the
RB> fd without deleting the event source object (I can't find a
RB> RemoveSourceForFd() function).

Actually there is, it's just implicit: the source is removed when it's
destroyed. I initially thought that we'd need a RemoveSource() method too
but now I remember that I decided against having it because the event loop
source dtor has to remove itself from the loop anyhow (sending
notifications to the already destroyed object wouldn't be a good idea) and
there doesn't seem to be any value in allowing to remove the source before
its destruction -- what use is a disconnected source?

So, to summarize, you just need to delete the source.

RB> I'm not sure that deleting the object will work here because I think
RB> this object is still used and needed after the child process is
RB> finished.

I don't see why should it be used after disabling the notifications? The
only purpose of this object is (or, at least, should be) to receive these
notifications, so why keep it afterwards?

RB> I also remember that if I don't disable the callbacks after we get EOF
RB> on stdout/stderr (wxExecute(SYNC)) that they keep getting called
RB> repeatedly, deadlocking the application.

This seems really, really strange to me. Did we discuss it already and I
just forgot it? IMO once we read the EOF from the FD, there should be no
more notifications.

RB> Therefore, I think I will be required to add the RemoveSourceForFd()
RB> function. If you agree, please give me your guidance on how you would
RB> like me to do that (function names, which class to put it in, etc).

If we really need it, it should be called RemoveSource() because it should
be usable for any kind of sources and it should go into wxEventLoopBase,
just as AddSourceForFD().

But I'd prefer to avoid this and rely on destroying the sources instead,
to avoid having zombie wxEventLoopSourceHandlers.


RB> VZ> And then change the process termination detection code to use SIGCHLD.
RB>
RB> I see that this is already done in your last commit, you took this from
RB> my patch. So am I missing something, or did you just forget that it is
RB> already done?

But I am not fully satisfied with how this is done in that "WIP" commit,
so I'd like to split it in several and do the switch to SIGCHLD as a
separate commit on top of the event loop sources changes in your branch.

Regards,
VZ

[Rob Bresalier]

Hi Vadim:

RB> But the library user can still call wxExecute(ASYNC) and create the
RB> event loop themselves after calling wxExecute(ASYNC).

 Let's just disallow this. It's not a huge thing to ask to create the event
loop before calling wxExecute(ASYNC), it's not like you're going to be able
to avoid doing it if you're using anything async.

 I.e. just insert a wxCHECK_MSG() in wxExecute() verifying that an event
loop is available.

My concern about disallowing this is backwards compatibility.  This was allowed before.  Is it a problem to disallow it now?  Could there be code out there using this and we will be breaking it?

Also, even if we did disallow this, how do I get a pointer or reference to the event loop object that the library user created?  I need this pointer/reference in order to call AddSourceForFd().

  

RB> I'm not sure that deleting the object will work here because I think
RB> this object is still used and needed after the child process is
RB> finished.

 I don't see why should it be used after disabling the notifications? The
only purpose of this object is (or, at least, should be) to receive these
notifications, so why keep it afterwards?

I will dig into the code to find out if there is any reason we need to keep the object around, and if so what I can move out of it into a different place.

 

RB> I also remember that if I don't disable the callbacks after we get EOF
RB> on stdout/stderr (wxExecute(SYNC)) that they keep getting called
RB> repeatedly, deadlocking the application.

 This seems really, really strange to me. Did we discuss it already and I
just forgot it? IMO once we read the EOF from the FD, there should be no
more notifications.

We did discuss this already and I think you forgot.  From what I recall, its just the way to OS/platform works. I distinctly remember debugging this and that I was required to shut down the notifications in order to stop repeatedly getting them at EOF.

 

RB> Therefore, I think I will be required to add the RemoveSourceForFd()
RB> function.  If you agree, please give me your guidance on how you would
RB> like me to do that (function names, which class to put it in, etc).

 If we really need it, it should be called RemoveSource() because it should
be usable for any kind of sources and it should go into wxEventLoopBase,
just as AddSourceForFD().

 But I'd prefer to avoid this and rely on destroying the sources instead,
to avoid having zombie wxEventLoopSourceHandlers.

It may be unavoidable to have this, but I will look into if it is possible to not need it.

 

RB> VZ> And then change the process termination detection code to use SIGCHLD.
RB>
RB> I see that this is already done in your last commit, you took this from
RB> my patch.  So am I missing something, or did you just forget that it is
RB> already done?

 But I am not fully satisfied with how this is done in that "WIP" commit,
so I'd like to split it in several and do the switch to SIGCHLD as a
separate commit on top of the event loop sources changes in your branch.

Can you be more specific about what you don't like about it?  I just don't see anything wrong with the code in your "WIP" commit and it works fine..

Regards,

Rob

[Vadim Zeitlin]

On Wed, 12 Jun 2013 14:46:30 -0400 Rob Bresalier wrote:

RB> RB> But the library user can still call wxExecute(ASYNC) and create the
RB> > RB> event loop themselves after calling wxExecute(ASYNC).
RB> >
RB> > Let's just disallow this. It's not a huge thing to ask to create the event
RB> > loop before calling wxExecute(ASYNC), it's not like you're going to be able
RB> > to avoid doing it if you're using anything async.
RB> >
RB> > I.e. just insert a wxCHECK_MSG() in wxExecute() verifying that an event
RB> > loop is available.
RB> >
RB> My concern about disallowing this is backwards compatibility. This was
RB> allowed before. Is it a problem to disallow it now? Could there be code
RB> out there using this and we will be breaking it?

I'm less concerned about breaking compatibility for the console
applications using event loops than for breaking it elsewhere because such
applications could only be written since 2.9.0, so I think there are not
that many of them. I also think it's rather unlikely, even if not
impossible, of course, that such an application would call wxExecute()
first and only create the event loop later -- this really doesn't seem to
make any sense.

RB> Also, even if we did disallow this, how do I get a pointer or reference to
RB> the event loop object that the library user created? I need this
RB> pointer/reference in order to call AddSourceForFd().

wxEventLoopBase::GetActive() should be used for this.

RB> > RB> I also remember that if I don't disable the callbacks after we get EOF
RB> > RB> on stdout/stderr (wxExecute(SYNC)) that they keep getting called
RB> > RB> repeatedly, deadlocking the application.
RB> >
RB> > This seems really, really strange to me. Did we discuss it already and I
RB> > just forgot it? IMO once we read the EOF from the FD, there should be no
RB> > more notifications.
RB> >
RB> We did discuss this already and I think you forgot. From what I recall,
RB> its just the way to OS/platform works. I distinctly remember debugging this
RB> and that I was required to shut down the notifications in order to stop
RB> repeatedly getting them at EOF.

Yes, I do remember already being surprised by this before. But I still
don't remember (and can't find, after rechecking right now) the exact
explanation of why is this needed. So I'd like to debug this myself to
understand it.

RB> > But I am not fully satisfied with how this is done in that "WIP" commit,
RB> > so I'd like to split it in several and do the switch to SIGCHLD as a
RB> > separate commit on top of the event loop sources changes in your branch.
RB> >
RB> Can you be more specific about what you don't like about it? I just don't
RB> see anything wrong with the code in your "WIP" commit and it works fine..

For the SIGCHLD part, I'd like to improve the following:

1. Get rid of static wxExecuteExecutedAlready (this is problematic because,
in principle, wx can be shut down and reinitialized, re-creating
wxTheApp too and this flag would get out of sync then). Instead, add
wxAppConsole::IsSignalHandlerSet() or something like this that would
check if the signal handler is already registered. The implementation
would, of course, be trivial as we already have m_signalHandlerHash
in wxAppConsole. And this could/should be a separate commit too.

2. I'd also like to do something about ChildProcessesOpenedHash. I'd like
to get rid of it too, of course (I like to make things simple if you
haven't noticed :-), but if we have it, it should be at least moved to
wxApp for the same reason as above.

Now why do I think it would be nice to remove it: I think it's confusing
to have a list of currently running child PIDs containing
wxEndProcessData and a list of child FD handlers in wxApp and also
wxExecuteData and also wxUnixProcess. The relationship between these
objects is not very clear at first glance (and probably not on the
second one neither).

Ideally I'd like to have just a single list containing all the data
associated with the child processes. I.e. merge wxEndProcessData (which
doesn't seem to be needed at all any more if we don't use it for the
process termination detection, actually) into wxExecuteData; make
wxUnixProcess part of it too (there really doesn't seem to be any need
to mention it at all in the public wxProcess interface) and keep all
wxExecuteData objects in a single list (whether it's a map or just a
vector) in wxApp.


This last simplification should be very nice, I think, and remove much of
the perceived complexity from the code. If we can have just one object,
instead of different ones accessed via different pointers from different
places, the code should become much simpler to understand. Because
fundamentally it really ought to be simple, there is nothing difficult in
keeping a list of child processes and generating a notification for its
element when our SIGCHLD handler gets called. Everything else is just
historical baggage that we should get rid of.


Thanks again,
VZ

[Rob Bresalier]

Hi Vadim:

My goal for my first commit/patch was to only change the FD handling to use wxEventLoop::AddSourceForFd() and to have a working implementation.

I was able to get this working on GTK, Cocoa and Carbon.  

I've attached a patch to the Trac, and also updated the code review:

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

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

I did not make any other changes, such as refactoring to join the wxExecuteData, wxEndProcessData and wxUnixProcess - that can be done in a later patch/commit.

However, I discovered some fundamental issues in OSX with the way AddSourceForFd() is implemented. I strongly recommend we switch its internals to use the socket method from my last wxExecute patch.  I will now describe to you in detail why I think this.

Taking a look inside of src\osx\core\evtloop_cf.cpp, we see that the OSX GUI implementation of AddSourceForFd() uses the CFFileDescriptorEnableCallBacks() API.  Here is a link to the documentation for that API:

http://developer.apple.com/library/mac/#documentation/CoreFoundation/Reference/CFFileDescriptorRef/Reference/reference.html

This API has a fatal flaw in it, quoted from the documentation:

"Each call back is one-shot, and must be re-enabled if you want to get another one."

This causes many complications for the implementation.  The 2 main ones are:

1) 

Inside of the pre-existing callback function wx_cffiledescriptor_callback() in evtloop_cf.cpp, the callbacks are re-enabled unconditionally at the end of the function by calling EnableDescriptorCallBacks() (which in turn calls CFFileDescriptorEnableCallBacks()).  I don't think this is proper behavior for the case where the callbacks are disabled inside the handler function (such as disabling the callback upon EOF detection).  Therefore, we should only re-enable the callback here if the event loop source has not been deleted.  But we have no easy way of knowing whether or not the event loop source was deleted.  I hacked up a way to know by saving this information in the handler object (you will see that in the patch), but it is messy.  Also, I think it can have problems if multiple event loop sources share the same handler.

2) 

When running the tests, I discovered that if the fd is handled by the wxSelectDispatcher (wxEXEC_NOEVENTS case), then when we run the next test that uses an event loop, we won't have the event fired off in the event loop and the test hangs.  The only way that I am able to get the event to fire off is if I re-enable the callbacks after running the OnReadWaiting() handler of the dispatcher.  This is really ugly (I hacked up an ugly solution just for demo purposes in the patch just to show you, of course I don't think it should be merged this way).

The socket based method used in my last wxExecute patch did not have these 2 issues, it is not one-shot and we did not need to keep re-enabling the callback.  Therefore, I propose that for my next commit that I re-implement AddSourceForFd() in OSX using the socket-based API (by copying the code from my last patch - see the work is not wasted!).

As far as you taking the time to debug why we keep getting events when we have hit EOF - I don't think it is worth your time.  We should be able to handle the case of the callback getting disabled from the handler before the return from wx_cffiledescriptor_callback(), even without this EOF case, so we need to solve this issue anyway.  Also, I think it is easier for the platforms (GTK and OSX) to always call the callback when EOF condition exists instead of detecting a change in this condition and only calling the callback then.  So I think it is normal and expected behavior.

Also - I really think that we should change all implementations of the AddSourceForFd() method into static methods.  It would be a more honest representation of what the function is really doing as it is implemented right now.  I reviewed the code for it in all platforms (GTK, OSX, Console), and I find that there is no data associated with the event loop instance that is impacted.  All platform API functions that are called are global to all event loops in the application.

Changing all wxEventLoop::AddSourceForFd() to a static method can clean up much code in this patch by avoiding the need to get an event loop object.  I've pointed out many places inside the patch (with code comments) that could be cleaned up if a change to static is made.

Also, by making it static, it could be possible to call wxExecute(ASYNC) without an existing event loop.  I know you said that it is not important, but I just wanted to point this out to you.

Making it a static method can also make the library user's life much simpler too, just like it would make the wxExecute() patch much cleaner - no need to get an existing event loop instance.

I volunteer to changing this to static method in a future commit if you agree.

Regards,

Rob

[Vadim Zeitlin]

On Mon, 17 Jun 2013 20:49:18 -0700 (PDT) Rob Bresalier wrote:

RB> Hi Vadim:
RB>
RB> My goal for my first commit/patch was to only change the FD handling to use
RB> wxEventLoop::AddSourceForFd() and to have a working implementation.

Hi again and thanks for resuming/continuing to work on this!

RB> I was able to get this working on GTK, Cocoa and Carbon.
RB>
RB> I've attached a patch to the Trac, and also updated the code review:
RB>
RB> http://trac.wxwidgets.org/ticket/10258
RB> http://review.bakefile.org/r/471

Thanks! Just a minor remark: it would probably be better to open a new
review for the next patch as this one starts getting too busy (the value of
reusing the same review for the consecutive versions of the same patch is
that RB allows you to view the inter-diff changes which is pretty nice, but
if a new patch is a different one and not just a new version of the old
one, this is not really relevant).

RB> I did not make any other changes, such as refactoring to join the
RB> wxExecuteData, wxEndProcessData and wxUnixProcess - that can be done in a
RB> later patch/commit.

Excellent, thanks a lot for doing it like this, this allowed me to review
the changes quickly and relatively easily.


RB> However, I discovered some fundamental issues in OSX with the way
RB> AddSourceForFd() is implemented. I strongly recommend we switch its
RB> internals to use the socket method from my last wxExecute patch.

If there are no drawbacks to using a socket, I'm fine with this, of
course. OTOH I wonder if we couldn't fix the current problem in a simpler
way bu re-enabling the callback before calling the overridden
OnReadWaiting()? It seems like it should take care of at least the first
problem.

RB> The socket based method used in my last wxExecute patch did not have these
RB> 2 issues, it is not one-shot and we did not need to keep re-enabling the
RB> callback. Therefore, I propose that for my next commit that I re-implement
RB> AddSourceForFd() in OSX using the socket-based API (by copying the code
RB> from my last patch - see the work is not wasted!).

Again, if there are no problems with doing this, I don't have any
objections. Unfortunately I don't know OS X API well enough to be sure that
there are no problems. It could be instructive to look at the other open
source projects and see what API do they use for similar things. If none of
them uses sockets, I'd hesitate to start using them... But hopefully this
is not the case.

RB> Also - I really think that we should change all implementations of the
RB> AddSourceForFd() method into static methods.

OK, let's do it, I think I already agreed to this in a previous
discussion.


I've left a few remarks concerning some of the changed of the patch but I
think all of them could be fixed rather easily/quickly. Please let me know
if you disagree or if you have any questions about what exactly did I mean.

And thanks again,
VZ

[Rob Bresalier]

Hi Vadim:

 Thanks! Just a minor remark: it would probably be better to open a new
review for the next patch as this one starts getting too busy (the value of
reusing the same review for the consecutive versions of the same patch is
that RB allows you to view the inter-diff changes which is pretty nice, but
if a new patch is a different one and not just a new version of the old
one, this is not really relevant).

Yes, I will create a new review.

For my next patch, do you want it to be a diff against my previous submission or a diff against your WIP commit?  I would think you want a diff against my last patch so it is a smaller package for you to consume, so you don't need to re-review code?  When we are done I can give you a patch that covers all the changes since your "WIP" commit.

RB> The socket based method used in my last wxExecute patch did not have these
RB> 2 issues, it is not one-shot and we did not need to keep re-enabling the
RB> callback.  Therefore, I propose that for my next commit that I re-implement
RB> AddSourceForFd() in OSX using the socket-based API (by copying the code
RB> from my last patch - see the work is not wasted!).

 Again, if there are no problems with doing this, I don't have any
objections. Unfortunately I don't know OS X API well enough to be sure that
there are no problems. It could be instructive to look at the other open
source projects and see what API do they use for similar things. If none of
them uses sockets, I'd hesitate to start using them... But hopefully this
is not the case.

Based on your suggestion, I did some searching around, and I did find usage of the Socket API in the Qt library for precisely the same function (monitoring file descriptors)

In Qt, there is the 'QSocketNotifier', which is used for monitoring file descriptors (not just sockets, but also pipes), see:

http://qt-project.org/doc/qt-4.8/qsocketnotifier.html

http://www.qtcentre.org/threads/18016-Watching-a-UNIX-file-descriptor

I looked into the source code for OSX, and it is using the OS X socket API:

See line 324 for the 'void QEventDispatcherMac::registerSocketNotifier(QSocketNotifier *notifier)' at this link:

https://qt.gitorious.org/+qt-s60-developers/qt/qt-s60/blobs/5356d3717a9355830a13dfa0c8726f86a5f1c522/src/gui/kernel/qeventdispatcher_mac.mm

I also found the API is used by google in the "Google toolbox for Mac", see:

http://google-toolbox-for-mac.googlecode.com/svn-history/r41/trunk/Foundation/GTMSignalHandler.m

So I think it is safe to use it.

In fact, I have coded it last night and it passes all the wxExecute tests on Cocoa, GTK, and Carbon.  And I was able to get rid of the hack code that I put in for demo purposes in order to get the code working with the old API!

RB> Also - I really think that we should change all implementations of the
RB> AddSourceForFd() method into static methods.

 OK, let's do it, I think I already agreed to this in a previous
discussion.

I started this work and ran into a complication.  Right now, there is a wxEventLoopBase::AddSourceForFd(), but there are also overloaded versions of these in the derived classes.  For static functions, there is no such thing as 'virtual static', so I needed to rename all of the versions in the derived classes (GUI, Console, and CF (OSX)) to new names (such as wxGUIEventLoop::AddSourceForFdGUI(), wxConsoleEventLoop::AddSourceForFdConsole(), and wxCFEventLoop::AddSourceForFdCF()).

Next is the problem of being able to call the right version based on whether we are a console or GUI application.

To do this, inside the body of the new static wxEventLoopBase::AddSourceForFd(), I get a pointer to the traits object (in the same way as wxExecute code), and then call a new function traits->AddSourceForFd().  I created GUI and console versions of wxAppTraits::AddSourceForFd(), and inside the appropriate version I call either the GUI (or CF for OSX) or console version.

But I'm not sure if this is the best way to do it, and wanted to ask you how you would do it.

I already have this coded and working, but it can be easily changed.

 

 I've left a few remarks concerning some of the changed of the patch but I
think all of them could be fixed rather easily/quickly. Please let me know
if you disagree or if you have any questions about what exactly did I mean.

I glanced over your remarks and I will start tackling them tonight.

Regards,

Rob

[Vadim Zeitlin]

On Wed, 19 Jun 2013 06:47:57 -0700 (PDT) Rob Bresalier wrote:

RB> For my next patch, do you want it to be a diff against my previous
RB> submission or a diff against your WIP commit?

Hi,

I think quite a few things are going to change compared to the last patch,
so perhaps a new diff against the commit before the WIP one (I think this
is what you meant, but just wanted to be precise) would be easier for both
of us.

RB> Based on your suggestion, I did some searching around, and I did find usage
RB> of the Socket API in the Qt library for precisely the same function
RB> (monitoring file descriptors)

OK, very good, thanks for checking.

RB> > RB> AddSourceForFd() method into static methods.
RB> >
RB> > OK, let's do it, I think I already agreed to this in a previous
RB> > discussion.
RB> >
RB> I started this work and ran into a complication. Right now, there is a
RB> wxEventLoopBase::AddSourceForFd(), but there are also overloaded versions
RB> of these in the derived classes. For static functions, there is no such
RB> thing as 'virtual static', so I needed to rename all of the versions in the
RB> derived classes (GUI, Console, and CF (OSX)) to new names (such as
RB> wxGUIEventLoop::AddSourceForFdGUI(),
RB> wxConsoleEventLoop::AddSourceForFdConsole(), and
RB> wxCFEventLoop::AddSourceForFdCF()).

To be honest I don't like these names very much. If we're going to use
wxAppTraits anyhow to virtualize these calls, why not just put the code
directly in wxAppTraits then?

RB> Next is the problem of being able to call the right version based on
RB> whether we are a console or GUI application.

This is what wxAppTraits is for, so it has to be used. The only question
is whether this should be done directly (as you are doing now, I think) or
indirectly, via another class, as it was done before with the concrete
wxEventLoop being created by wxAppTraits and then all the event loop stuff
done in this derived wxEventLoop class.

RB> But I'm not sure if this is the best way to do it, and wanted to ask you
RB> how you would do it.

If we do it like this, I'd just prefer to keep the real implementation in
wxAppTraits itself and make wxEventLoopBase::AddSourceForFD() just a
trivial wrapper.

This is not very elegant, it would probably be nicer to do it indirectly
by adding some new wxEventLoopFactory class which would actually represent
the class of wxEventLoop, thus making it possible to have "virtual static":
wxEventLoop methods which would be simple virtual methods of this class.

I.e., in pseudo code:

// New class in wx/private/eventloopfactory.h
class wxEventLoopFactoryBase {
public:
virtual wxEventLoopBase* CreateLoop() = 0;
#if wxUSE_EVENTLOOP_SOURCE
virtual wxEventLoopSource* AddSourceForFD(int fd, wxEventLoopSourceHandler *handler, int flags) = 0;
#endif
};

// Changed code in wx/apptrait.h
class wxAppTraitsBase {
public:
...

// This replaces the old CreateEventLoop().
virtual wxEventLoopFactoryBase *GetEventLoopFactory() = 0;

// We may keep this one for convenience and to avoid
// updating the existing code inside wx itself.
virtual wxEventLoopBase *CreateEventLoop()
{
// This shouldn't be inline as we don't have
// full wxEventLoopFactoryBase declaration
// here, but in src/common/appbase.cpp
return GetEventLoopFactory()->CreateLoop();
}
};

// Implementation in e.g. src/gtk/evtloop.cpp (moved from
// src/gtk/utilsgtk.cpp), will be similar for the other ports
class wxGTKEventLoopFactory : public wxEventLoopFactoryBase {
public:
virtual wxEventLoopBase *CreateEventLoop()
{
return new wxGUIEventLoop();
}

virtual wxEventLoopSource* AddSourceForFD(int fd, wxEventLoopSourceHandler *handler, int flagsint fd, wxEventLoopSourceHandler *handler, int flagsint fd, wxEventLoopSourceHandler *handler, int flags)
{
... real code using g_io_add_watch() goes here ...
}
};

// This can be just a global object, there doesn't seem to be
// any benefit in allocating it on the heap.
static wxGTKEventLoopFactory gs_eventLoopFactory;

wxEventLoopFactoryBase *wxGUIAppTraits::GetEventLoopFactory()
{
return &gs_eventLoopFactory;
}

/* static */
wxEventLoopSource*
wxGUIEventLoop::AddSourceForFD(int fd, wxEventLoopSourceHandler *handler, int flags)
{
return gs_eventLoopFactory.AddSourceForFD(fd, handler, flags);
}


Having written this just now, it doesn't seem that bad actually and looks
nicer than just stuffing all this into wxAppTraits. So I think it would be
nicer to do it like this. What do you think?



RB> > I've left a few remarks concerning some of the changed of the patch but I
RB> > think all of them could be fixed rather easily/quickly. Please let me know
RB> > if you disagree or if you have any questions about what exactly did I
RB> > mean.
RB> >
RB> I glanced over your remarks and I will start tackling them tonight.

Thanks again!
VZ

[Rob Bresalier]

Hi Vadim:

I need to digest the details in your mail, but want to comment on one item quickly while our time zones overlap.

 I think quite a few things are going to change compared to the last patch,
so perhaps a new diff against the commit before the WIP one (I think this
is what you meant, but just wanted to be precise) would be easier for both
of us.

No, I mean your WIP commit, not the one before it.  There is much code that you put in there that is needed (many things unrelated to how fds are monitored).  To be precise, I'm referring to all the changes you have in here:

https://github.com/vadz/wxWidgets/commit/5f951773390b38a0347c601f01ee8277c7935ba9

My changes are built on top of the changes in this commit, this is where I branched off from you in my local git repository.

Maybe you have 1 extra commit in your local repository that you never pushed to github?

Regards,

Rob

[Rob Bresalier]

I used this commit of yours because it is a fully working version, so I can use it to verify changes work.

I know you want more refactoring, such as joining wxExecuteData, wxUnixProcess, wxEndProcess (may not be the exact names), so I'll do that in a later commit and remove code from your WIP commit to do it.  This way, I can start with something that passes all tests, refactor that, and know if I broke anything.

[Vadim Zeitlin]

On Wed, 19 Jun 2013 07:29:09 -0700 (PDT) Rob Bresalier wrote:

RB> I used this commit of yours because it is a fully working version, so I can
RB> use it to verify changes work.

OK, I didn't even realize this, sorry. As usual, ideal would be to have
small obviously correct changes, e.g. the first one adding
wxEventLoopFactory (if we go that way), then others building on top of it.
But I can do the rebasing myself once we have the final version of the
patch.

Thanks,
VZ

[Rob Bresalier]

I was thinking that for the next patch I give you, with the wxEventLoopFactory (if we go that way, still need to look at that tonight, outside my day job), will be against the WIP commit, as we just discussed.  I also plan to have the OS X API change to sockets in that commit as I've already done it, and it impacts only 1 file and is pretty localized.

But then, when I do the refactoring to merge the data structures - I will start where I left off, but then I can make the patch be a diff against the commit just before the WIP commit.  Then, it may be easier for you to understand, as you won't see code that is ripped out and replaced, you'll just see all the wxExecute stuff as new code.

[Rob Bresalier]

Hi Vadim:

I have working versions (passing the tests) for GTK, Cocoa, Carbon and unix console using your wxEventLoopFactory proposal (with some changes detailed below).  I've also incorporated your comments in the review - and responded back to some of them.  One thing is still a little unclear - about the handling of HUP condition in GTK - see the comments in the review.

I still need to clean up what I have done before submitting a new patch/review for you to look at.  Hopefully I will have time for that tomorrow night.

Here are some responses to your previous mail:

 

 I.e., in pseudo code: 

        // New class in wx/private/eventloopfactory.h 
        class wxEventLoopFactoryBase { 
        public: 
                virtual wxEventLoopBase* CreateLoop() = 0; 

I don't think 'CreateLoop' is required for this patch.  Its probably a nice to have.  I want to avoid doing it because it could impact more than the Unix ports.  It could be added on top of my addition of wxEventLoopFactory.

 

                // We may keep this one for convenience and to avoid 
                // updating the existing code inside wx itself. 
                virtual wxEventLoopBase *CreateEventLoop() 
                { 
                        // This shouldn't be inline as we don't have 
                        // full wxEventLoopFactoryBase declaration 
                        // here, but in src/common/appbase.cpp 
                        return GetEventLoopFactory()->CreateLoop(); 
                } 
        }; 

This is also not required for this patch, and I will avoid doing it.  It could be added later to the library.  I'll keep the old version of this function for now.

 

        // Implementation in e.g. src/gtk/evtloop.cpp (moved from 
        // src/gtk/utilsgtk.cpp), will be similar for the other ports 

For GTK, I was able to successfully implement this in evtloop.cpp instead of utilsgtk.cpp.

But it is a different story for OSX port.  In OSX, the evtloop_cf.cpp is compiled with -DwxUSE_GUI=0 -DwxUSE_BASE=1'.  This makes it impossible to reference the wxGUIAppTraits, I get compile errors that wxGUIAppTraits is unrecognized.  Therefore in OSX port, I had to put this code into utilsexc_cf.cpp, which is compiled with '-DwxUSE_BASE=0'. Then it compiles and passes the tests.

 

        /* static */ 
        wxEventLoopSource* 
        wxGUIEventLoop::AddSourceForFD(int fd, wxEventLoopSourceHandler *handler, int flags) 
        { 
                return gs_eventLoopFactory.AddSourceForFD(fd, handler, flags); 
        } 

I think you made a mistake here.  The static function should only be wxEventLoopBase::AddSourceForFD(), and not in wxGUIEventLoop.  And I think it needs a traits pointer.  I have a working/compiled implementation of this static function (inside src/common/evtloopcmn.cpp) and it looks like this:

#if wxUSE_EVENTLOOP_SOURCE

wxEventLoopSource* wxEventLoopBase::AddSourceForFD(int fd, wxEventLoopSourceHandler *handler, int flags)

{

    wxConsoleAppTraits traitsConsole;

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

    if ( !traits )

        traits = &traitsConsole;

    return traits->GetEventLoopFactory()->AddSourceForFD(fd,handler,flags);

}

#endif

Regards,

Rob

[Vadim Zeitlin]

On Wed, 19 Jun 2013 21:44:51 -0700 (PDT) Rob Bresalier wrote:

RB> I have working versions (passing the tests) for GTK, Cocoa, Carbon and unix
RB> console using your wxEventLoopFactory proposal (with some changes detailed
RB> below).

Great!

RB> I've also incorporated your comments in the review - and responded
RB> back to some of them. One thing is still a little unclear - about the
RB> handling of HUP condition in GTK - see the comments in the review.

I am not sure about it neither but it seems to make more sense to map
G_IO_HUP to our wxEVENT_SOURCE_INPUT rather than wxEVENT_SOURCE_EXCEPTION
as EOF is not really an error.

RB> > I.e., in pseudo code:
RB> >
RB> > // New class in wx/private/eventloopfactory.h
RB> > class wxEventLoopFactoryBase {
RB> > public:
RB> > virtual wxEventLoopBase* CreateLoop() = 0;
RB>
RB> I don't think 'CreateLoop' is required for this patch.

Not really required, but without it the name wxEventLoopFactory doesn't
make any sense as "factory" is used for the classes responsible for
creating things. The trouble is that I have no idea about what to call this
class if it's only responsible for adding the sources to the event loops.
wxEventLoopSourcesManager? "Manager" is just a weasel word, of course, it
only fits here because it can mean absolutely anything (and hence means
nothing). But I just don't see any other candidates.

RB> > // Implementation in e.g. src/gtk/evtloop.cpp (moved from
RB> > // src/gtk/utilsgtk.cpp), will be similar for the other ports
RB> >
RB>
RB> For GTK, I was able to successfully implement this in evtloop.cpp instead
RB> of utilsgtk.cpp.
RB>
RB> But it is a different story for OSX port. In OSX, the evtloop_cf.cpp is
RB> compiled with -DwxUSE_GUI=0 -DwxUSE_BASE=1'. This makes it impossible to
RB> reference the wxGUIAppTraits, I get compile errors that wxGUIAppTraits is
RB> unrecognized. Therefore in OSX port, I had to put this code
RB> into utilsexc_cf.cpp, which is compiled with '-DwxUSE_BASE=0'. Then it
RB> compiles and passes the tests.

OK, it's not that important to move it to the event loop file, I just
thought it would be natural to do it.

RB> > /* static */
RB> > wxEventLoopSource*
RB> > wxGUIEventLoop::AddSourceForFD(int fd, wxEventLoopSourceHandler
RB> > *handler, int flags)
RB> > {
RB> > return gs_eventLoopFactory.AddSourceForFD(fd, handler,
RB> > flags);
RB> > }
RB> >
RB> I think you made a mistake here. The static function should only be
RB> wxEventLoopBase::AddSourceForFD(), and not in wxGUIEventLoop.

Yes, right, sorry.

RB> And I think it needs a traits pointer. I have a working/compiled
RB> implementation of this static function (inside
RB> src/common/evtloopcmn.cpp) and it looks like this:
RB>
RB> #if wxUSE_EVENTLOOP_SOURCE
RB> wxEventLoopSource* wxEventLoopBase::AddSourceForFD(int fd,
RB> wxEventLoopSourceHandler *handler, int flags)
RB> {
RB> wxConsoleAppTraits traitsConsole;
RB> wxAppTraits *traits = wxTheApp ? wxTheApp->GetTraits() : NULL;
RB> if ( !traits )
RB> traits = &traitsConsole;
RB>
RB> return traits->GetEventLoopFactory()->AddSourceForFD(fd,handler,flags);
RB> }
RB> #endif

This looks good, thanks!
VZ

[Rob Bresalier]

Hi Vadim:

I posted to the same review (http://review.bakefile.org/r/471/) because I got an error when trying to create a new review.  I was able to post to the old review without error.

The patch has 3 main changes in it:

- Made wxEventLoopBase::AddSourceForFD a static method

- Changed OSX implementation of AddSourceForFD to socket based API

- Modified wxExecute patch to use AddSourceForFD

I also made updates based on your last review comments.

Its compiling and passing tests on GTK, Cocoa, Carbon (both GUI and console on these 3 platforms).

Below is the command I ran to create a new review and the corresponding error message.

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

There was an error creating this review request.

The repository path "https://github.com/vadz/wxWidgets.git" is not in the

list of known repositories on the server.

Ask the administrator to add this repository to the Review Board server.

For information on adding repositories, please read

http://www.reviewboard.org/docs/manual/dev/admin/configuration/repositories/

------

Looking forward to your comments.

Regards,

Rob

[Vadim Zeitlin]

On Thu, 20 Jun 2013 21:11:01 -0700 (PDT) Rob Bresalier wrote:

RB> I posted to the same review (http://review.bakefile.org/r/471/) because I
RB> got an error when trying to create a new review. I was able to post to the
RB> old review without error.

Hello,

I think you need to set the REPOSITORY environment variable for
post-review, i.e. do

REPOSITORY=https://svn.wxwidgets.org/svn/wx post-review -g

RB> The patch has 3 main changes in it:
RB>
RB> - Made wxEventLoopBase::AddSourceForFD a static method
RB> - Changed OSX implementation of AddSourceForFD to socket based API
RB> - Modified wxExecute patch to use AddSourceForFD
RB>
RB> I also made updates based on your last review comments.
RB>
RB> Its compiling and passing tests on GTK, Cocoa, Carbon (both GUI and console
RB> on these 3 platforms).

Excellent, thanks, I'll look at it a.s.a.p. (possibly tomorrow though) and
will leave my comments there -- and/or just commit it if I don't see any
real problems.

Thanks again,
VZ