GNOME Bugzilla – Bug 109966
Dispatcher does not work on win32
Last modified: 2007-01-24 13:10:22 UTC
Glib::Dispatcher builds fine on win32 using mingw32 but the examples/thread/dispatcher and dispatcher2 examples do not work.
I received a workaround a few days ago from Andrew Makeev. I somewhat rearranged the code and adapted it to the current CVS dispatcher.cc. A patch is attached. The dispatcher examples now run fine. If somebody (Daniel ?) could have a look at it... I'm not a pipe/file descriptor specialist ! If it's ok, I'll add a patched ChangeLog.
Created attachment 15466 [details] [review] Dispatcher win32 patch
Platform-specific workarounds _must_ have source code comments explaining what they do and why they are necessary.
As I said, I got this win32-specific Dispatcher from Andrew Makeev without any further explanation. Having no experience with pipe/file descriptor, I decided to just create a patch and submit it AS IS on bugzilla for review/comments by specialists. Maybe if Andrew or Daniel could comment on this...
Yes, I was just making clear what needs to be done before we can commit it. Thanks for making sure that it didn't get lost.
Created attachment 15696 [details] [review] Added commentaries to the patch
Created attachment 15730 [details] [review] fixed lock for prev patch
Great. Even though it still doesn't mean much to me, that's a lot better. However, I don't know what "btw" means - maybe that could be changed? It seems like this is ready for Daniel to look at it. Daniel?
Daniel is hard to contact recently. Do tell us if you require this in a release urgently.
btw = between, just common abbreviation. The patch is needed by our Application that we are developing at the moment, thus patching every new release before installing and building RPM's is very annoying, you know.
I have cleaned this up a bit and committed it to both branches. Please try to conform to existing styles when patching documents. Thanks a lot for this. Please check that it still works as expected.
The attached patch fixes a small typo in glib/glibmm/dispatcher.cc. Without it, the file does not compile on win32. As a side note, examples/thread/dispatcher freezes on my machine (win2k + SP3). The dispatcher2 example is fine.
Created attachment 16365 [details] [review] dispatcher patch for win32 (correct a small typo)
Applied to both branches. Thanks. Actually, I thought you had cvs write access now - feel free to commit simple stuff like this.
Reopened because parts of the new win32 code look fishy to me. For instance, the DispatchNotifyData objects in the queue are both read and written without any locking at all. Only locking the queue is not sufficient, you have to lock the data too. I think it'd be best to get rid of of dynamic memory allocation altogether -- there's simply no need for this. Further, there is absolutely no need for Locked_Queue<> to be a template, and neither should all methods be inlined. Other issues: DispatchNotifier::create_pipe() calls g_win32_error_message() whereas warn_failed_pipe_io() uses g_strerror(), so I guess the latter is incorrect. Also, the win32 code should pass the name of the function which returned the error to warn_failed_pipe_io().
Created attachment 27890 [details] [review] fixed-up patch It would be nice if someone could try the new patch on Win32. I hope it compiles cleanly; right now I have now way to test glibmm on Win32.
I tried the new dispatcher against the current glibmm CVS on win32 (mingw-gcc 2.3.2). dispatcher.cc and the two dispatcher examples compile fine. The dispatcher2.exe example runs as expected. The other example (dispatcher.exe) crashes with a segmentation fault when the last (5th) thread reaches 100% (finished). I'm investigating...
Status?
I found the bug in the dispatcher.cc example. It is related to the use of auto_ptr. In the on_progress_finish method, a ThreadProgress pointer was removed from progress_list_ AND the corresponding object was deleted, which caused some kind of race condition on win32. To solve this problem, I removed the auto_ptr's. RefPtr's to the ThreadProgress objects are now stored in a separate list (progress_ref_list_), which means that the ThreadProgress objects are deleted when the Application destructor is called. I think this is also a nice example of a RefPtr used with a non Glib::Object (see the reference and unreference methods in ThreadProgress). dispatcher now runs fine on win32, without memory leak. If somebody could have a look at the code, test it on linux and comment...
Created attachment 30390 [details] [review] Fixes crash of dispatcher example on win32
Somebody had time to try my latest patch ? Should I commit it to CVS ?
Please ask for testers on the list, and then commit after a couple of days if there are no problems reported (even if nobody tests it). I can't do it right now. Thankyou.
It seems like time to commit this.
Commited and (hopefully) resolved.
I accidentally found the real cause of the race condition Cedric experienced. It was actually a bug in my patch, not in the example. Also, I just reworked the win32 implementation somewhat (sticking closely to msdn) in order to make it match the Dispatcher behavior on Unix more closely. The code compiles fine with dummy definitions of the win32 API functions, but I haven't tested yet whether it works at runtime. Cedric, if (and only if) you have the time, I'd appreciate if you could try whether current glibmm/trunk works for you on win32. I could also send you a patch instead if that would make matters easier for you.
I tested your changes to dispatcher on the glibmm-2.12 svn branch. The library builds fine with mingw32. examples/thread/dispatcher.cc also returns the expected result. I guess it would be interesting to commit those changes to glibmm-2.12 as well.
Many thanks, Cedric. I'm glad that it works. Could you also please try whether it works correctly with the call to Glib::usleep() in examples/thread/dispatcher.cc commented out? If it does, I'll commit the changes to glibmm-2.12, too :-)
Oh, and while you're at it, could you please also try with the Glib::usleep() replaced by Glib::Thread::yield()? Many thanks, again :-)
Attached are the outputs of the dispatcher example with Glib::usleep(), Glib::Thread::yield or none of the two.
Created attachment 81065 [details] Output of dispatcher example with neither usleep nor yield
Created attachment 81066 [details] Output of dispatcher example with usleep
Created attachment 81067 [details] Output of dispatcher example with yield