On 25 September 2012 17:29, Kevin Ollivier <
kev...@theolliviers.com> wrote:
> The simple fact is that events are fired by controls when a call to one of their
> methods or their own handlers induces a state change. Events are not all the
> same, and sometimes, you can do things like skip an event or veto / cancel
> an event, which will cause OS level handlers to apply some default behavior
> when control returns to them.
Yes you are right, it is more difficult than I had imagined. I am now
convinced that trying to automatically defer the destroy until 'safe'
is too difficult to attempt in one patch, or in a point release, or
perhaps ever. Thank you for taking the time to help me grok that.
It still leaves the option to raise an exception when Destroy is
called in an event handler, which is an option I have offered before,
and the only option to receive a +1 so far.
> So to use the set focus example again, do you feel control.SetFocus, or all
> user code that uses it, should take into account you may destroy the control
> from within that method? Because code like that fires events, which then
> fires your handler, and it is all synchronous, meaning that when you get
> EVT_SET_FOCUS, you are still inside SetFocus, or more accurately, still
> inside the OS equivalent call for SetFocus.
Well, yes I think it should. Whenever a library calls user callbacks
it must check it's preconditions again. I'm sure the OS apis do this.
Take win32 for example, it never assumes that a HWND is valid, it
always checks and returns failure if it is invalid.
> When running your tests, there are a variety of factors like this that you need
> to consider when designing a proper test. And because wxWidgets (and
> wxPython) are largely different codebases on Win, GTK and Mac, there are
> certainly cases that will crash one but not the others. That is precisely why
> library developers are generally skittish about changing a long-standing core
> method's behavior on all platforms.
Which is totally understandable when you are changing working
behaviour to a different working behaviour. But going from crashing
to non-crashing is not, IMO one that I would not be skittish about.
> If the OS is still accessing it, it most certainly will unless they check the
> OS object for validity prior to each call to it. (Obj-C/Cocoa has that
> feature built-in, but I've rarely seen other toolkits do that, though
> certain ref-counting systems do.) This is not something you can
> verify by running one test, just as calling Destroy does not lead to
> problems in all cases, or even most cases.
My tests on the other frameworks suggest that they all check this. As
any responsible API would do which has callbacks and a Destroy method.
> You call GUI code after the GUI is already in an indeterminate state, and just expect it will work and not cause further problems?
It was only in an indeterminate state because a bug in agw and/or wx
caused a spurious mouse event to occur. (Which we have worked
around).
On this occasion that bug caused Destroy to be called by agw code from
an event handler and crashed. But many other times it wouldn't have
caused Destroy and the indeterminate state may well have gone
undetected, as would be the case for many other classes of bugs in wx
code, agw code, or our code. But whatever the cause, I don't think
we should use Destroy crashing as a litmus test for the application
being in a determinate state as that would only catch a minority of
them anyway.
In my application it would always be better to have a spurious mouse
event to occur than a crash. It would be impossible (in our
application) for that spurious mouse event to do anything that the
user could not put right in seconds - i.e. a single mouse event never
does anything particularly powerful, and the undo buffer could have
resolved it quickly. But a crash could easily cost the user hours of
work. It's the lesser of two evils for our application, you'll just
have to take my word for that. YMMV of course.
Unfortunately we have experienced quite a lot of weird bugs like this
in wxPython and though it got a lot better in 2.9, it also introduced
many more with it. So we test, a lot. It was during testing we found
this one I'm glad to say, but it could have ended up on a customers
desktop before being discovered.
In any case, if Destroy had raised an exception and not crashed we
would have been able to handle it much better.
> Again, Destroy will only crash in certain circumstances, and when it
> does there is a problem in its calling code somewhere. You can
> disagree with this if you like, but this much I can say is true.
I never argued otherwise. Though I could - the docs actively suggest
that you could, and other frameworks allow it.
> I realize you're more concerned about Destroy not crashing than getting
> your problem fixed, but getting your problem fixed is the sure-fire
> accurate and reliable way to keep Destroy from crashing!
I have fixed/worked around that particular problem, both Destroy and
the click through are in my bug tracker to fix, I am discussing the
Destroy fix now because it is relevant to this thread topic.
>> 2) That I would heuristically guess whether an event handler was
>> active, and that I would get false positives. I told you that I wasn't
>> doing this.
>
> Come back and re-write that once you've verified that you can never,
> under any circumstance, using any OS event, get Destroy to crash any
> longer.
Come back and rewrite what? That I wasn't using a heuristic mechanism
to detect being in an event handler? I will write that many times as
you like. That's irrelevant to Destroy being never able to crash.
This argument seems to lead to "Let's not fix anything unless we can
fix everything". I never claimed to fix them all, but if we can fix
10% of them, then we can look at the next 10%.
>> This new concern about OS objects is valid, and requires some testing
> It's not a new concern, it's the core of my argument
Honestly, you hadn't mentioned this before, if you disagree then
please can you post a link to the precise message where you did? I am
at a loss as to what might possibly have been alluding to it.
> and the entire
> reason I say what you're doing is a heuristic, because, as I said before,
> your counter isn't going to the OS level, so it will sometimes be right,
> sometimes not, depending on the native OS handling of the event.
There are two issues to debate here, I'll separate them so that we can
discuss them more clearly.
1 Can we detect that Destroy has been called from an event handler?
2 What should we do if we do detect this?
1 Can we detect that Destroy has been called from an event handler?
Perhaps I misunderstand the way the events reach python code. Don't
all events that end up in python go through
wxAppConsoleBase::CallEventHandler? If they do all go through there,
and the counting is done there, then how could the counter be wrong?
As far as I can imagine, we're not concerned with events that don't go
to python because then the python code can't call Destroy, and here we
are concerned with cases where the python calls Destroy.
Are you concerned with false positives or negatives, just to be clear:
false positives : we think we're in an event handler but we're not
false negatives : we think we're not in an event handler but actually we are
false positives would be very bad as it would prevent legitimate calls
to Destroy, and that would be a show stopper.
false negatives would be a shame because it would mean that
illegitimate calls to Destroy would be allowed, leading to some
crashes that would have happened before, still crashing now, but not a
showstopper.
IMO there would be neither false positive nor false negatives, but
clearly there are people on this list who understand all this better
than I do.
> In any case, if it is indeed necessary to defer to later than the
> end of the event in some cases, and I do believe it is, we
> should recommend the same fix for all cases to be safe. And,
> that fix is simple, of course - wx.CallAfter. If you're going to
> alter Destroy and do it right, you might as well just change
> wxPython's Destroy to use wx.CallAfter. Though again, for the
> same reasons stated before, a change to when Destroy takes
> effect probably won't be accepted because it changes existing
> code from "destroy now" to "destroy at some indeterminate
> point in the future", which depending on when that "indeterminate
> point" is, could lead to unexpected, and not always reproducible,
> runtime behavior changes with existing code. That's the other
> major reason your fix is unlikely to be accepted, and this one I
> can ensure you I've pointed out repeatedly.
Yes you have repeatedly pointed out that the fix I have never
suggested would never be accepted.
2 What should we do if we (can and) do detect this?
I suggested two options for what could happen when a Destroy was
called in an event handler
1. defer the Destroy (either with CallAfter or sooner if it were
possible, it seems that this is not practical however)
2. raise an exception
I know you are against option 1. Are you also against option 2?
I also suggested this could be a runtime option, with either 1 or 2 as
the default. Are you against this as well?
> And perhaps that is the most important point to press. The fact that
> changing Destroy behavior silently at runtime for all wxPython apps
> (and perhaps you even intend the change for all wx apps, since your
> counting is happening in wx C++)
The detection code would be in wx I think as it is easier to achieve
there, but what to do about it when detected would only be in
wxPython. I would let the wxWidgets developers decide whether they
wanted to act on this, or whether they want to compile out the code
which allows the detection so they don't have to pay the negligible
performance penalty.
It would only affect those wxPython apps which we agree need to be fixed anyway.
> is going to require an immense amount of testing to ensure the change
> is truly safe,
I would think that any improvement in safety, would be, well, an improvement.
The side effects depend on the course of action chosen.
> and I doubt anyone would really feel that the effort is worth
> preventing the occasional crash when things go wonky. Particularly
> when such improvement can't even be precisely measured, since we
> don't even yet know the actual reason that Destroy is crashing for you
> in the first place and hence don't know whether or not your fix would
> actually improve or even change anything at all in that case.
I have posted example code that several people have reproduced as
crashing, is that not enough?
If the code doesn't fix anything of course there is no point at all is there.
Sam