Opened 12 years ago

Closed 11 years ago

Last modified 10 years ago

#10320 closed defect (fixed)

Selective Yield implementation

Reported by: frm Owned by: frm
Priority: critical Milestone: 2.9.0
Component: GUI-generic Version: stable-latest
Keywords: yield, selective, event, category, execute, sync Cc: rk@…, al@…
Blocked By: Blocking:
Patch: yes

Description

This patch is the result of the "Yield like problems" thread on wx-dev.

Please note that it's not complete yet:
1) tested only on wxGTK

2) many GetEventCategory() are missing; it's important to decide what wxEvent::GetEventCategory() should return before adding one GetEventCategory() in every derived event class

3) there are possible problems with wxEvtHandler::ProcessEvent() return value; see my wx-dev message http://article.gmane.org/gmane.comp.lib.wxwidgets.devel/109529

4) wx global lists should probably be removed and made private wxAppBase's members to avoid possible breakage by the users

5) we may need to alter wxApp::Yield implementation when wxEVT_CATEGORY_USER_INPUT flags are absent (see Robin's message).

Attachments (5)

selective_yield.patch download (43.6 KB) - added by frm 12 years ago.
selective_yield.2.patch download (43.7 KB) - added by frm 12 years ago.
selective_yield.3.patch download (43.6 KB) - added by frm 12 years ago.
selective_yield.4.patch download (61.2 KB) - added by frm 12 years ago.
app_to_evtloop.patch download (92.7 KB) - added by frm 12 years ago.
implements step 2: wxApp=>wxEvtLoop function moving

Download all attachments as: .zip

Change History (26)

Changed 12 years ago by frm

comment:1 Changed 12 years ago by frm

  • Milestone set to 2.9.0
  • Owner set to frm
  • Priority changed from normal to critical
  • Status changed from new to accepted
  • Version set to 2.9-svn

I'm updating the patch against trunk.
It would be nice to receive feedback on this (possibly big) patch until I have all fresh in my mind so I can complete it.

Changed 12 years ago by frm

comment:2 follow-up: Changed 12 years ago by vadz

I think it would be nicer to leave the old Yield(bool onlyIfNeeded = false) and add an overload Yield(long categories) (or maybe even a differently named YieldFor()) because I think the onlyIfNeeded is a band aid and shouldn't be used at all.

Other than that I don't have any specific comments about the patch. I dislike the way it makes the code in ProcessPendingEvents() (which already had many bugs in it...) even more complicated but I don't see how can this be avoided unfortunately.

It would be really nice if more people could test this patch before it is applied. TIA!

comment:3 in reply to: ↑ 2 Changed 12 years ago by frm

Replying to vadz:

I think it would be nicer to leave the old Yield(bool onlyIfNeeded = false) and add an overload Yield(long categories) (or maybe even a differently named YieldFor()) because I think the onlyIfNeeded is a band aid and shouldn't be used at all.

ok; I think a YieldFor() function would be a good name.

Other than that I don't have any specific comments about the patch.

there are various open points about it however.
Mails where I describe them are:

http://article.gmane.org/gmane.comp.lib.wxwidgets.devel/109529
http://article.gmane.org/gmane.comp.lib.wxwidgets.devel/109625

and in particular:
http://article.gmane.org/gmane.comp.lib.wxwidgets.devel/109672

Link to the entire thread:
http://thread.gmane.org/gmane.comp.lib.wxwidgets.devel/109242
(from which I've extracted the mails linked above)

I dislike the way it makes the code in ProcessPendingEvents() (which already had many bugs in it...) even more complicated but I don't see how can this be avoided unfortunately.

see http://article.gmane.org/gmane.comp.lib.wxwidgets.devel/109672 for the proposal of _not_ modifying ProcessEvent() but rather modify wxEvtHandler::ProcessPendingEvents() and the wx code which synthetize (wxWidgets) events from the native events.

It would be really nice if more people could test this patch before it is applied. TIA!

After the points I've listed in these mails are solved, I'll prepare a "final version" of this patch and will test it accurately!

Thanks!

comment:4 Changed 12 years ago by frm

Ok, this time I've attached a complete version of the patch, which can be applied to the trunk.

As discussed in the "wxYield again" thread on wx-dev, I've reconsidered the initial decision of filtering events at wx-level. This is not possible because we cannot return "I don't want to process it now; delay it to later". It's instead possible to selectively process events from the native message queues.

Notes about the patch:

  • does really implements the event filtering for wxMSW and wxGTK only; adds a TODO comment for other ports
  • replaces wxYield() in GTK's clipboard code with a wxTheApp->YieldFor() call, thus fixing possible reentrancies (and modifies clipboard sample to test synchronous IsSupported calls)
  • replaces wxYieldIfNeeded() calls in wxProgressDialog with wxTheApp->YieldFor() calls, so that it processes only UI/user-input events, thus fixing the race condition of the "thread" sample (but does not sets TEST_YIELD_RACE_CONDITION==1 since it wouldn't make the sample "intuitive")
  • it has been tested on wxGTK and wxMSW only; may break compilation of other ports

Changed 12 years ago by frm

Changed 12 years ago by frm

comment:5 follow-up: Changed 12 years ago by vadz

Thanks, I see it more clearly now.

One minor naming remark: the functions should be called Suspend/ResumeProcess_ing_OfPendingEvents() and not SuspendProcessOfEvents().

One possibly less minor remark: could all this be done at wxEventLoop level rather than in wxApp? It seems like it belongs there and not in wxApp itself.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 12 years ago by frm

Replying to vadz:

One minor naming remark: the functions should be called Suspend/ResumeProcess_ing_OfPendingEvents() and not SuspendProcessOfEvents().

fixed in my local copy :)

One possibly less minor remark: could all this be done at wxEventLoop level rather than in wxApp? It seems like it belongs there and not in wxApp itself.

indeed it should.

If it's okay, I'd proceed in this way:

1) apply the patch as currently is (except for Process => Processing)

2) move all the function bodies of the following wxApp* classes:

virtual void SuspendProcessingOfPendingEvents();
virtual void ResumeProcessingOfPendingEvents();
virtual void ProcessPendingEvents();
bool HasPendingEvents() const;
virtual bool Pending();
virtual bool Dispatch();
virtual bool ProcessIdle();
bool Yield(bool onlyIfNeeded = false)
bool YieldFor(long eventsToProcess)
virtual bool IsYielding() const
virtual bool IsEventAllowedInsideYield(wxEventCategory WXUNUSED(cat)) const

to wxEventLoop leaving in wxApp some simple "redirections" to wxEventLoop methods (some of them already are simple redirections).
This is because both yielding and pending events stuff should go in wxEventLoop and not directly in wxApp.

3) change the remaining wxYield[IfNeeded] calls scattered in

  • wxGenericTreeCtrl drag-and-drop code
  • wxTopLevelWindowGTK::RequestUserAttention
  • wxSplashScreen
  • wxListMainWindow::EditLabel
  • wxDataViewMainWindow::OnRenameTimer

with YieldFor(wxEVT_CATEGORY_UI).
I wouldn't touch

  • wxGUIAppTraits::WaitForChild
  • wxPostScriptPrinter::Print and some other places in printing code.

since I don't know what they yield for and probably yielding for GUI events there is wrong...

comment:7 in reply to: ↑ 6 Changed 12 years ago by frm

Replying to frm:

1) apply the patch as currently is (except for Process => Processing)

this was done.

2) move all the function bodies of the following wxApp* classes:

virtual void SuspendProcessingOfPendingEvents();
virtual void ResumeProcessingOfPendingEvents();
virtual void ProcessPendingEvents();
bool HasPendingEvents() const;
virtual bool Pending();
virtual bool Dispatch();
virtual bool ProcessIdle();
bool Yield(bool onlyIfNeeded = false)
bool YieldFor(long eventsToProcess)
virtual bool IsYielding() const
virtual bool IsEventAllowedInsideYield(wxEventCategory WXUNUSED(cat)) const

to wxEventLoop leaving in wxApp some simple "redirections" to wxEventLoop methods (some of them already are simple redirections).
This is because both yielding and pending events stuff should go in wxEventLoop and not directly in wxApp.

Ok, I'm attaching a patch which does this. I understand it's difficult to revise it (it's more than 3000 lines long), so I'll make here a short log of what it effectively does:

  • moves the function list above to wxEvtLoop
  • adds simple redirections to wxAppBase
  • changes docs accordingly
  • removes the global list wxPendingEvents (and its mutex wxPendingEventsLocker): this is a backward incompatible change motivated by:

a) few apps (aMule, skinning addon libraries,...) use the list directly and precisely they only use wxPendingEventsLocker for stopping processing of pending events at will; they will thus need to change their code to use wxEvtLoopBase::GetActive()->SuspendProcessinfOfPendingEvents() instead.
b) now that we have a new list to store the event handlers which do have pending events, but which cannot processed now (because they do not match the Yield() filter), then if an app uses wxPendingEvents (and not just the locker) directly then it would subtly broken (no compiler error - possible misbehaviour at runtime difficult to spot). This is because apps built for wx28 have no knowledge of the new list for delayed events handlers
c) an untyped global wxList is bad.
d) adding a typed member list of wxEvtLoopBase with simple accessors makes the code simpler&safer

The patch was tested on wxGTK,wxMSW,wxMotif. Compilation of wxX11 was tested.

If the patch is OK I can commit it myself... hope to receive comments (thanks Vadim! ;)) before it gets out-of-sync.

Changed 12 years ago by frm

implements step 2: wxApp=>wxEvtLoop function moving

comment:8 follow-up: Changed 12 years ago by vadz

Sorry, I don't have time to really review it once again but some random comments after reading the description and glancing over the patch:

  1. Please describe the backwards-incompatible changes in docs/changes.txt succinctly (i.e. "wxPendingEvents removed, use SuspendProcessinfOfPendingEvents" instead) and in docs/doxygen/overviews/backwardcompatibility.h in more details.
  2. wxEventLoop::DoYield() is new so we don't have any compatibility constraints and should get rid of "bool onlyIfNeeded" parameter -- please make it a flag, I even think we could combine it with "eventsToProcess". BTW, do we even need still this at all? At the very least, it seems we don't need it in DoYield() as we have IsYielding() which could be tested by the public Yield().
  3. There is some commented out code in wxEventLoopBase::ProcessPendingEvents(), could you please clean it up.
  4. It would be better if posting events still worked even without/before the event loop is started, this seems like something you could very well do inadvertently.
  5. It would be nice to have more detailed description of this code logic somewhere (in wxEventLoopBase declaration maybe?) as I'm not sure to grasp it.

Thanks!

comment:9 in reply to: ↑ 8 Changed 11 years ago by frm

Replying to vadz:

Sorry, I don't have time to really review it once again but some random comments after reading the description and glancing over the patch:

  1. Please describe the backwards-incompatible changes in docs/changes.txt succinctly (i.e. "wxPendingEvents removed, use SuspendProcessinfOfPendingEvents" instead) and in docs/doxygen/overviews/backwardcompatibility.h in more details.

done

  1. wxEventLoop::DoYield() is new so we don't have any compatibility constraints and should get rid of "bool onlyIfNeeded" parameter -- please make it a flag, I even think we could combine it with "eventsToProcess". BTW, do we even need still this at all? At the very least, it seems we don't need it in DoYield() as we have IsYielding() which could be tested by the public Yield().

Indeed. I've renamed DoYield() methods to YieldFor() then and moved the code which tests for recursive Yield() calls to wxEventLoopBase::Yield().

  1. There is some commented out code in wxEventLoopBase::ProcessPendingEvents(), could you please clean it up.

ops; done.

  1. It would be better if posting events still worked even without/before the event loop is started, this seems like something you could very well do inadvertently.

I don't know how this could be done however... and OTOH having a debug message if such thing happens may help to spot some unwanted behaviour.

  1. It would be nice to have more detailed description of this code logic somewhere (in wxEventLoopBase declaration maybe?) as I'm not sure to grasp it.

done.

I'm going to commit this patch as soon as I've finished testing it.

comment:10 Changed 11 years ago by frm

For the reference: see commits r58654 and r58911 for the first two parts of this patch.

comment:11 Changed 11 years ago by FM

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [58916]) use YieldFor() in wxTopLevelWindowGTK::RequestUserAttention; comment on the use of YieldFor() in wxProgressDialog; add some TODOs near wxYieldIfNeeded()/wxSafeYield() calls in wxListCtrl, wxDataViewCtrl, wxTreeCtrl (closes #10320)

comment:12 follow-up: Changed 11 years ago by rk

  • Cc rk@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened

The code changes in r58911 introduced several dead-locks in my formerly working application.

The first problem is an actual bug in wxEventLoopBase::ProcessPendingEvents(). The critical section statements inside the while loop are in the wrong order.

void wxEventLoopBase::ProcessPendingEvents()
{
    wxENTER_CRIT_SECT(m_handlersWithPendingEventsLocker);

    ...

    while (!m_handlersWithPendingEvents.IsEmpty())
    {
        wxENTER_CRIT_SECT(m_handlersWithPendingEventsLocker);   <--- should be wxLEAVE_...

        m_handlersWithPendingEvents[0]->ProcessPendingEvents();

        wxLEAVE_CRIT_SECT(m_handlersWithPendingEventsLocker);   <--- should be wxENTER_...
    }

    ...

    wxLEAVE_CRIT_SECT(m_handlersWithPendingEventsLocker);
}

The next problem could be my not-understanding if the logging system is supposed to be thread-safe or not. In my application I have two threads. If the second thread wants to output a log message I do it like this:

wxString str("Some message");

wxCommandEvent event( wxEVT_MY_LOG_EVENT );
event.SetString( str.c_str() );
event.SetInt( tracelevel );
m_logWindow->GetEventHandler()->QueueEvent( new wxCommandEvent(event) );

In the gui-thread I connect to wxEVT_MY_LOG_EVENT and put the message into my log window. My log class is also derived from wxLog and set as the active target in order to get normal log messages from the gui-thread.

void Logger::onLogEvent( wxCommandEvent& event )
{
    m_textCtrl->AppendText(event.GetString());
}

void Logger::DoLogString(const wxString& msg, time_t timestamp)
{
    m_textCtrl->AppendText(msg);    <---- dead-lock
}

This all worked fine until r58911. Now my application hangs in Logger::DoLogString() because of a wxLogDebug call in wxEvtHandler::QueueEvent().

void wxEvtHandler::QueueEvent(wxEvent *event)
{
    wxEventLoopBase* loop = wxEventLoopBase::GetActive();
    if (!loop)
    {
        wxLogDebug("No event loop is running!");  <--- is this allowed from another thread?
        return;
    }

    ...
}

So the question is: should I make my logger thread-safe or should wxEvtHandler::QueueEvent() not call wxLogDebug() from a non-gui thread?

The last problem is that queued events don't seem to be deleted anymore. I get a lot of memory leaks now from the wxCommandEvents that I created in my second thread.

Once again those problems were not there before r58911.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 11 years ago by frm

Replying to rk:

The first problem is an actual bug in wxEventLoopBase::ProcessPendingEvents(). The critical section statements inside the while loop are in the wrong order.

I've fixed this in r58992. Sorry!

The next problem could be my not-understanding if the logging system is supposed to be thread-safe or not.

...

So the question is: should I make my logger thread-safe or should wxEvtHandler::QueueEvent() not call wxLogDebug() from a non-gui thread?

good question. Vadim, the wxLog class docs and wxLog overview do not contain the word "thread". We need to document (and eventually fix) if wxLog is thread-safe or not.

@rk: while I understand that the problem is caused by the wxLogDebug call, I don't understand why you get a dead-lock: what's the condition for which your two threads wait endlessly? Is there a mutex involved? If yes, which one?

The last problem is that queued events don't seem to be deleted anymore. I get a lot of memory leaks now from the wxCommandEvents that I created in my second thread.

this looks strange since when ~wxEventPtr in wxEvtHandler::ProcessPendingEvents() is called it should remove the queued event.
Are you using Yield calls or wx-code which calls Yield?
Could you try to put a breakpoint on the DelayPendingEventHandler() call in ProcessPendingEvents() and see if it ever gets executed?

In any case I'll try to reproduce the memleak in my code.

Many thanks for the report!
Francesco

comment:14 in reply to: ↑ 13 ; follow-up: Changed 11 years ago by rk

Replying to frm:

Replying to rk:

The first problem is an actual bug in wxEventLoopBase::ProcessPendingEvents(). The critical section statements inside the while loop are in the wrong order.

I've fixed this in r58992. Sorry!

Those things happen. ;-)

@rk: while I understand that the problem is caused by the wxLogDebug call, I don't understand why you get a dead-lock: what's the condition for which your two threads wait endlessly? Is there a mutex involved? If yes, which one?

The non-gui thread hangs in a SendMessage call (I am on msw, btw) inside wxTextCtrl::GetNumberOfLines which indirectly comes from wxTextCtrl::AppendText(). The gui thread hangs in a WaitForMultipleObjects call originating from a boost::condition variable. The problem now is that SendMessage blocks because it waits for the gui-thread to process the message. But the gui thread is waiting in the condition variable and therefor does not come back to the message loop. So the wxLog call might have actually worked if my gui-thread would just process its messages.

The last problem is that queued events don't seem to be deleted anymore. I get a lot of memory leaks now from the wxCommandEvents that I created in my second thread.

this looks strange since when ~wxEventPtr in wxEvtHandler::ProcessPendingEvents() is called it should remove the queued event.
Are you using Yield calls or wx-code which calls Yield?
Could you try to put a breakpoint on the DelayPendingEventHandler() call in ProcessPendingEvents() and see if it ever gets executed?

It is actually simpler than that. The memory leak comes from wxEvtHandler::QueueEvent. When there is no event loop present it just returns without deleting the event object. I can only see this right now because I commented out the wxLogDebug call.

comment:15 in reply to: ↑ 14 ; follow-up: Changed 11 years ago by frm

Replying to rk:

@rk: while I understand that the problem is caused by the wxLogDebug call, I don't understand why you get a dead-lock: what's the condition for which your two threads wait endlessly? Is there a mutex involved? If yes, which one?

The non-gui thread hangs in a SendMessage call (I am on msw, btw) inside wxTextCtrl::GetNumberOfLines which indirectly comes from wxTextCtrl::AppendText().

indeed. I think that your Logger class should do:

void Logger::onLogEvent( wxCommandEvent& event )
{
    if (wxThread::IsMain())
        m_textCtrl->AppendText(event.GetString());
}

void Logger::DoLogString(const wxString& msg, time_t timestamp)
{
    if (wxThread::IsMain())
        m_textCtrl->AppendText(msg);    <---- dead-lock
}

because otherwise you may call a GUI function (wxTextCtrl::AppendText) from a secondary thread. Basically the problem arises from the fact that before my change in wxEvtHandler::QueueEvent, you never found wxWidgets code which called a wxLog function from a secondary thread... now such code is there and thus your logger class needs to be thread-aware.

I don't know if we want to change this... without that wxLogDebug call it may be much more difficult for developers to understand what's going on when the event loop is not running. And the problem you describe only happens if you have a custom wxLog-derived class which is not thread-aware... maybe this is the "less worse".

The last problem is that queued events don't seem to be deleted anymore. I get a lot of memory leaks now from the wxCommandEvents that I created in my second thread.

this looks strange since when ~wxEventPtr in wxEvtHandler::ProcessPendingEvents() is called it should remove the queued event.
Are you using Yield calls or wx-code which calls Yield?
Could you try to put a breakpoint on the DelayPendingEventHandler() call in ProcessPendingEvents() and see if it ever gets executed?

It is actually simpler than that. The memory leak comes from wxEvtHandler::QueueEvent. When there is no event loop present it just returns without deleting the event object.

thanks for pointing this out. I've fixed it in r59037.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 11 years ago by rk

  • Resolution set to fixed
  • Status changed from reopened to closed

Replying to frm:

indeed. I think that your Logger class should do:
...
because otherwise you may call a GUI function (wxTextCtrl::AppendText) from a secondary thread. Basically the problem arises from the fact that before my change in wxEvtHandler::QueueEvent, you never found wxWidgets code which called a wxLog function from a secondary thread... now such code is there and thus your logger class needs to be thread-aware.

I agree that the logger should be thread-aware and I am going to make it that way. Now that log functions are called from other threads the documentation should say something about that so that others don't run into the same problem.

I don't know if we want to change this... without that wxLogDebug call it may be much more difficult for developers to understand what's going on when the event loop is not running. And the problem you describe only happens if you have a custom wxLog-derived class which is not thread-aware... maybe this is the "less worse".

I see no problem in leaving the wxLogDebug call there now that I know how to handle the logging. OTOH, the current system did not help me as developer. Because I never got the message that was supposed to tell me what was going wrong. ;-)
But again, if the documentation tells people how to handle logging in different threads it would probably not be a problem. This calls for a new ticket to get the documentation of wxLog updated.

I am closing this ticket now because the problems I had are solved. Thanks.

comment:17 in reply to: ↑ 16 Changed 11 years ago by frm

Replying to rk:

Replying to frm:

indeed. I think that your Logger class should do:
...
because otherwise you may call a GUI function (wxTextCtrl::AppendText) from a secondary thread. Basically the problem arises from the fact that before my change in wxEvtHandler::QueueEvent, you never found wxWidgets code which called a wxLog function from a secondary thread... now such code is there and thus your logger class needs to be thread-aware.

I agree that the logger should be thread-aware and I am going to make it that way. Now that log functions are called from other threads the documentation should say something about that so that others don't run into the same problem.

Please note that I've discovered that wxLog classes aren't thread safe currently.
This poses a threat for MT apps which incurr in the wxEvtHandler::QueueEvent's call to wxLogDebug (but this isn't critical: it only happens for apps which don't have a running event loop -- and that probably needs to be changed to have one).

We're looking to implement thread-safe logging in #10508, so I'll leave this ticket closed.

I don't know if we want to change this... without that wxLogDebug call it may be much more difficult for developers to understand what's going on when the event loop is not running. And the problem you describe only happens if you have a custom wxLog-derived class which is not thread-aware... maybe this is the "less worse".

I see no problem in leaving the wxLogDebug call there now that I know how to handle the logging. OTOH, the current system did not help me as developer. Because I never got the message that was supposed to tell me what was going wrong. ;-)

true... but as I said this is probably the "less worse" ;)

But again, if the documentation tells people how to handle logging in different threads it would probably not be a problem. This calls for a new ticket to get the documentation of wxLog updated.

Indeed #10508 will need docs updated, too.

Thanks for your feedback!

comment:18 Changed 11 years ago by FM

(In [59284]) move pending event processing back to wxApp (these methods were moved into wxEventLoopBase during YieldFor() refactoring - see #10320): we need to be able to queue events even when there's no event loop running (e.g. wxApp::OnInit)

comment:19 Changed 11 years ago by alarsen

  • Cc al@… added
  • Keywords execute sync added
  • Resolution fixed deleted
  • Status changed from closed to reopened

I just noticed that wxExecute(...wxEXEC_SYNC...) never returns on wxGTK.

Bisecting the problem I found that r58654 which mentions this ticket (but didn't compile) or r58655 is the culprit.

The bug can be easily reproduced with the tread sample; just press F5 (and on the latest version click OK)

My configuration is --enable-compat28 --enable-stl --enable-stc --without-gnomeprint --enable-shared --disable-debug on Linux 2.6.24/x86_64, gtk 2.12.9

Cheers
Anders

comment:20 Changed 11 years ago by frm

  • Status changed from reopened to confirmed

I can confirm this bug.
It's also something surely related to the selective yield implementation since placing the

        while (gtk_events_pending())
            gtk_main_iteration();

lines in wxGUIAppTraits::WaitForChild() replacing the wxYield() call fixes the problem.

I've already find a problem with wxEVT_CATEGORY_ALL which didn't include all event categories (see r59536) but it's not enough to fix this bug... :/

comment:21 Changed 11 years ago by FM

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [59541]) implement the wxGTK selective yield with a different approach: rather than getting the events ourselves and fetching them to gtk_main_do_event(), install our own wxgtk_main_do_event() to filter them (closes #10320 -- see bug reported in comment 19)

Note: See TracTickets for help on using tickets.