Author Topic: wx2.8.12, sending event cross threads are not safe in current CB source  (Read 29912 times)

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 6035
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
There are two functions which can send events from a worker thread to main GUI.
wxPostEvent() and AddPendingEvent()

According to the discussions below:

I think there are many issues in our C::B source:

wxCommandEvent is not thread safe for its m_cmdString member, e.g.:
Code
            
            wxString message;
            wxCommandEvent evt(...);
            evt.SetString(message.c_str());
            wxPostEvent(m_Parent, evt);
The code snippet is not thread safe, because "evt" and the cloned wxCommandEvent shared the same wxString member buffer.

CodeBlocksEvent is not thread safe, e.g. in cbthreadpool.cpp
Code
      CodeBlocksEvent evt = CodeBlocksEvent(cbEVT_THREADTASK_ALLDONE, m_ID);
      wxPostEvent(m_pOwner, evt);
CodeBlocksEvent is a derived class from wxCommandEvent, and I don't see it Clone its m_strString.

3, I'm not sure PipedProcess class has such issue, because its related to send event cross processes.

Solution:
If the event need to carry wxString from worker thread to main GUI thread, we should implement a Clone method which deep copy its m_cmdString. Just like the new event type wxThreadEvent in wx 2.9.x.

Any comments.



If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9702
Solution:
If the event need to carry wxString from worker thread to main GUI thread, we should implement a Clone method which deep copy its m_cmdString. Just like the new event type wxThreadEvent in wx 2.9.x.
Yes, we should always do. As a matter of fact, for wx events its a good thing to always implement the clone method once you are using member vars.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 6035
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Ok, this is the first draft patch. I add a new class named cbThreadEvent, which replace all the cross thread sending message from wxCommandEvent to cbThreadEvent. The patch also included an improved hook measure patch from: Performance measurement of all C::B event handlers
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9702
Ok, this is the first draft patch.
Could you do a proper SVN based patch? This patch does not apply.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline golgepapaz

  • Multiple posting newcomer
  • *
  • Posts: 44
This ,
Could you do a proper SVN based patch? This patch does not apply.

Won't it be proper that defining the event in sdk_events.h and sdk_events.cpp instead of
cclogger.cpp and it's own header?

Also about that profiling stuff, I don't want to pay for something that I don't need. This should be
optional.


Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9702
Won't it be proper that defining the event in sdk_events.h and sdk_events.cpp instead of
cclogger.cpp and it's own header?
Its not meant to be captured form outside. CC is a plugin, not the SDK. So the event is in the correct place.

Also about that profiling stuff, I don't want to pay for something that I don't need. This should be
optional.
Its only for debugging atm and surely will be in #defines to be explicitly compiled in or even removed after all. Patches posted here are fr playing with and will be cleaned-up before application. Don't you worry...
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline golgepapaz

  • Multiple posting newcomer
  • *
  • Posts: 44
Its not meant to be captured form outside. CC is a plugin, not the SDK. So the event is in the correct place.
I think it more of a general-purpose event than something exclusive to CC as the name
and the purpose suggest. Since we are stuck with wx2.8.12 for some time, I consider it a utility for
for developers who cannot use wxThreadEvent (I myself need it for example). There are many utility classes in the sdk like nullptr_t
cbThreadPool, cbAssert etc. I don't see why this should be different.

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 6035
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Note my patch is only a draft patch, which show the direction, but it surely need all your comments. So, thanks for all your comments.
@golgepapaz
I think a good place to put the new event in sdk_events.h and sdk_events.cpp. But for me, I just put it in a new file, because once I change the sdk_events.h, then I need to rebuild all my targets..... That's some times waste a lot of time.

I believe that sending event cross thread not only happens in CC.
About the performance measure on the event hooks, that's my direction to find a "hang" bug, because sometimes, my Codeblocks will hang for 2 or 3 seconds, so I would like to see which cause this hang issue. :)

@Morten, this is the patch against SVN.
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9702
@Morten, this is the patch against SVN.
OK, thanks for that - it applied safely.

However, having this applied the Spellchecker plugin does not compile anymore. The reason is that is derives directly from HookFunctorBase and therefore your newly introduced method "GetTypeName2 is not implemented as it is pure virtual.

This is (however) most likely a design issue in the SpellChecker plugin. I don't know the rationale for deriving directly from this class (probably due to the need to override "Call"?!), but IMHO that should not be done and needed.

Maybe danselmi can enlighten us...?! ;-)

EDIT: And its the same for the OccurrenceHighlighter plugin... :-/
« Last Edit: June 25, 2013, 09:55:55 pm by MortenMacFly »
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Is OccurrenceHighlighter still needed? I don't see in the contrib subdir...
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline danselmi

  • Developer
  • Almost regular
  • *****
  • Posts: 205
This is (however) most likely a design issue in the SpellChecker plugin.

Corrected in rev. 9164

Offline danselmi

  • Developer
  • Almost regular
  • *****
  • Posts: 205
Is OccurrenceHighlighter still needed? I don't see in the contrib subdir...

OccurrenceHighlighter is an addition to the occurrence highlighting implemented in the core. It is able to permanently (until the string gets removed from the list again) highlight strings in all editors. Not only the currently selected.

Should I add it into contrib subdir?

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9702
Should I add it into contrib subdir?
I think so, yes (in fact I was under the assumption its there already...). I find it quite useful!
And thanks for the fix, btw...
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Should I add it into contrib subdir?
I'd say core and also extract the other highlight occurrence code from the sdk in it.
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 6035
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Note my patch is only a draft patch, which show the direction, but it surely need all your comments. So, thanks for all your comments.
@golgepapaz
I think a good place to put the new event in sdk_events.h and sdk_events.cpp. But for me, I just put it in a new file, because once I change the sdk_events.h, then I need to rebuild all my targets..... That's some times waste a lot of time.

I believe that sending event cross thread not only happens in CC.
Now, I add the class implementation in a cpp file.
This is a new patch ( I add a new cpp file in the SDK)
See attachment, any comments?
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.