There are two functions which can send events from a worker thread to main GUI.
wxPostEvent() and AddPendingEvent()
According to the discussions below:
- wx forum post: thread safety issue about wxCommandEvent in wx2.8.12 (http://forums.wxwidgets.org/viewtopic.php?f=1&t=37553&sid=15e20872c551c3d02ffad001890b8633)
- our CB forum post: unnecessory locker in CClogger (http://forums.next.codeblocks.org/index.php/topic,18059.0.html)
I think there are many issues in our C::B source:
wxCommandEvent is not thread safe for its m_cmdString member, e.g.:
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
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.
Quote from: ollydbg on June 21, 2013, 02:32:41 AM
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.
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 (http://forums.next.codeblocks.org/index.php/topic,17974.0.html)
Quote from: ollydbg on June 21, 2013, 09:34:31 AM
Ok, this is the first draft patch.
Could you do a proper SVN based patch? This patch does not apply.
This ,
Quote from: MortenMacFly on June 21, 2013, 01:13:34 PM
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.
Quote from: golgepapaz on June 21, 2013, 03:55:31 PM
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.
Quote from: golgepapaz on June 21, 2013, 03:55:31 PM
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...
Quote from: MortenMacFly on June 21, 2013, 04:17:42 PM
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.
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.
Quote from: ollydbg on June 21, 2013, 05:34:54 PM
@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... :-/
Is OccurrenceHighlighter still needed? I don't see in the contrib subdir...
Quote from: MortenMacFly on June 25, 2013, 09:54:12 PM
This is (however) most likely a design issue in the SpellChecker plugin.
Corrected in rev. 9164
Quote from: oBFusCATed on June 26, 2013, 12:15:11 AM
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?
Quote from: danselmi on June 26, 2013, 12:58:52 AM
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...
Quote from: danselmi on June 26, 2013, 12:58:52 AM
Should I add it into contrib subdir?
I'd say core and also extract the other highlight occurrence code from the sdk in it.
Quote from: ollydbg on June 21, 2013, 05:34:54 PM
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?
I still thinkthat the event does not need its own header and implementation file.sdk_events will do fine. That profiling
stuff is still not optional and please don't forget to update the other project configurations . (If you are commited to
going with a separate file please also add header file to the projects for the sake of convention.)
Quote from: golgepapaz on July 22, 2013, 06:50:02 PM
I still thinkthat the event does not need its own header and implementation file.sdk_events will do fine.
OK, I agree with you, and I will change the code.
Quote
That profiling
stuff is still not optional and please don't forget to update the other project configurations . (If you are commited to
going with a separate file please also add header file to the projects for the sake of convention.)
OK, I will add the preprocessor directive, and do my best, note I only have a windows and wx2.8.12, so adding to other project files may cause some build issue. :)
OK, this is the new patches, by using the "git rebase -i" tool, I can nicely split one commit to two. (Git is wonderful, see how to split a commit here: version control - How to split last commit into two in Git - Stack Overflow (http://stackoverflow.com/questions/1440050/how-to-split-last-commit-into-two-in-git))
The first patch introduce the performance hook measurement, it use preprocessor directive now, see first attachment.
The second patch introduce the CodeBlocksThreadEvent class, and use it.
BTW: The patch is generate by GIT command like:
$ git format-patch -M -C --output-directory /f/cb_sf_git/patches master...90654
dbd50701149639
Quote from: ollydbg on July 23, 2013, 08:26:37 AM
OK, this is the new patches, by using the "git rebase -i" tool, I can nicely split one commit to two. (Git is wonderful [...]
...except that both patches (as expected) do
not apply to our SVN source tree. >:(
Quote from: MortenMacFly on July 24, 2013, 07:44:39 AM
Quote from: ollydbg on July 23, 2013, 08:26:37 AM
OK, this is the new patches, by using the "git rebase -i" tool, I can nicely split one commit to two. (Git is wonderful [...]
...except that both patches (as expected) do not apply to our SVN source tree. >:(
OK, I will create the svn patches soon.
Question: How do you apply patches, for the git style patch, you can just run the command in your svn local copy.
patch <./xxx.patch
Note, I'm using the patch utility from MSYS.
EDIT: I'm not sure how to create two continuous patches in SVN, for example, 001.patch, 002.patch, but 002.patch can only applied after 001.patch.
Quote from: ollydbg on July 24, 2013, 07:47:49 AM
Note, I'm using the patch utility from MSYS.
None of the version of patch for Windows ever worked for me. Yiannis used to send me a very old version which was working, hence it got lost on the way... (and it was incompatible with Windows Vista+ to my knowledge...)
Quote from: MortenMacFly on July 24, 2013, 07:50:57 AM
Quote from: ollydbg on July 24, 2013, 07:47:49 AM
Note, I'm using the patch utility from MSYS.
None of the version of patch for Windows ever worked for me. Yiannis used to send me a very old version which was working, hence it got lost on the way... (and it was incompatible with Windows Vista+ to my knowledge...)
I just successfully apply the git style patches in my svn local copy, then create two svn patches, see attachment. The second patch was a combination of 0001 and 0002 patch.
You can try a modern MSYS, I suggest you can try this: http://msysgit.googlecode.com/files/PortableGit-1.8.3-preview20130601.7z, it should works fine.
EDIT: I'm not saying a standalone patch utility(I never tried it), I'm saying the patch utility in MSYS.
FYI: I commit the two patches in the trunk(rev9310, rev9311).