View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0035395 | Lazarus | Widgetset | public | 2019-04-16 10:04 | 2019-12-13 23:19 |
| Reporter | Boris Glavin | Assigned To | Juha Manninen | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | resolved | Resolution | reopened | ||
| Platform | Linux | OS | All | OS Version | All |
| Product Version | 2.1 (SVN) | Product Build | |||
| Target Version | Fixed in Version | 2.0.4 | |||
| Summary | 0035395: Race condition in TGtk2WidgetSet (TGtkMessageQueue) | ||||
| Description | In methods Lock/UnLock of TGtkMessageQueue ``` procedure TGtkMessageQueue.Lock; begin inc(fLock); if fLock=1 then {$IFDEF USE_GTK_MAIN_OLD_ITERATION} EnterCriticalsection(FCritSec); {$ELSE} g_main_context_acquire(FMainContext); {$ENDIF} end; procedure TGtkMessageQueue.UnLock; begin dec(fLock); if fLock=0 then {$IFDEF USE_GTK_MAIN_OLD_ITERATION} LeaveCriticalsection(FCritSec); {$ELSE} g_main_context_release(FMainContext); {$ENDIF} end; ``` race status is possible | ||||
| Steps To Reproduce | Execute multiple call PostMessage from another threads | ||||
| Additional Information | It is possible to add critical section for solution of the problems: ``` procedure TGtkMessageQueue.Lock; begin EnterCriticalSection(FLockCritSec); inc(fLock); if fLock=1 then {$IFDEF USE_GTK_MAIN_OLD_ITERATION} EnterCriticalsection(FCritSec); {$ELSE} g_main_context_acquire(FMainContext); {$ENDIF} LeaveCriticalSection(FLockCritSec); end; procedure TGtkMessageQueue.UnLock; begin EnterCriticalSection(FLockCritSec); dec(fLock); if fLock=0 then {$IFDEF USE_GTK_MAIN_OLD_ITERATION} LeaveCriticalsection(FCritSec); {$ELSE} g_main_context_release(FMainContext); {$ENDIF} LeaveCriticalSection(FLockCritSec); end; ``` | ||||
| Tags | No tags attached. | ||||
| Fixed in Revision | r61026, r62219 | ||||
| LazTarget | - | ||||
| Widgetset | GTK 2 | ||||
| Attached Files |
| ||||
|
|
Please create valid patch |
|
|
Does it need another critical section? Or would a InterLockedIncrement() / InterLockedDecrement() do? |
|
|
Yes, InterlockedIncrement/InterlockedDecrement will solve a problem with in excess calls of g_main_context_release. Patch added. |
|
|
gtk2msgqueue.path (683 bytes) |
|
|
gtk2msgqueue1.path (683 bytes) |
|
|
Macroes LOCK_CONTEXT/UNLOCK_CONTEXT - thread save? https://github.com/GNOME/glib/blob/master/glib/gmain.c InterlockedIncrement/InterlockedDecrement not working |
|
|
Please test, and close if ok. |
|
|
My old patch does not solve a problem because it is deeper. The call of g_main_context_iteration removes blocking and leads to a race during adding of events in queue. It is necessary to think and understand what the author of a solution tried to make. An example for reproduction of an error in the attached archive. In the analysis of the code only what g_main_context_acquire/g_main_context_release it makes sense to cause only from the main flow is evident (is caused implicitly through TGtkMessageQueue.Lock/TGtkMessageQueue.UnLock in TGtk2WidgetSet.AppProcessMessages). The call from other flows g_main_context_acquire/g_main_context_release does not make any sense (it is a part of glib, and GTK not thread safe). gtk2msgqueue3.path (1,357 bytes) test.zip (128,468 bytes) |
|
|
Update path and debug project testgtk2msgqueue.zip (3,691 bytes) gtk2msgqueue4.path (2,354 bytes) |
|
|
re-open, and unassigned => needs to go to one of the gtk maintainers |
|
|
Boris, please give your patches a suffix ".patch" instead of ".path". They can be viewed directly from a web browser when you do so. |
|
|
ok
gtk2msgqueue4.patch (2,354 bytes)
Index: lcl/interfaces/gtk2/gtk2msgqueue.pp
===================================================================
--- lcl/interfaces/gtk2/gtk2msgqueue.pp (revision 61030)
+++ lcl/interfaces/gtk2/gtk2msgqueue.pp (working copy)
@@ -50,12 +50,12 @@
TGtkMessageQueue=class(TLinkList)
private
FPaintMessages: TDynHashArray; // Hash for paint messages
+ FCritSec: TRTLCriticalSection;
{$IFNDEF USE_GTK_MAIN_OLD_ITERATION}
FMainContext: PGMainContext;
{$ELSE}
- FCritSec: TRTLCriticalSection;
+ fLock: integer;
{$ENDIF}
- fLock: integer;
protected
function CreateItem : TLinkListItem;override;
function CalculateHash(ParWnd : Hwnd):integer;
@@ -86,6 +86,9 @@
implementation
+uses
+ Classes;
+
{---(TGtkMessageQueueItem)----------------------}
function TGtkMessageQueueItem.IsPaintMessage: Boolean;
@@ -121,9 +124,8 @@
inherited Create;
FPaintMessages := TDynHashArray.Create(-1);
FPaintMessages.OwnerHashFunction := @HashPaintMessage;
- {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
InitCriticalSection(FCritSec);
- {$ELSE}
+ {$IFNDEF USE_GTK_MAIN_OLD_ITERATION}
FMainContext := g_main_context_new;
g_main_context_ref(FMainContext);
{$ENDIF}
@@ -133,31 +135,36 @@
begin
inherited Destroy;
fPaintMessages.destroy;
- {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
- DoneCriticalsection(FCritSec);
- {$ELSE}
+ {$IFNDEF USE_GTK_MAIN_OLD_ITERATION}
g_main_context_unref(FMainContext);
FMainContext := nil;
{$ENDIF}
+ DoneCriticalsection(FCritSec);
end;
procedure TGtkMessageQueue.Lock;
begin
+ {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
if InterlockedIncrement(fLock)=1 then
- {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
EnterCriticalsection(FCritSec);
{$ELSE}
- g_main_context_acquire(FMainContext);
+ if GetCurrentThreadId = MainThreadID then
+ g_main_context_acquire(FMainContext)
+ else
+ EnterCriticalsection(FCritSec);
{$ENDIF}
end;
procedure TGtkMessageQueue.UnLock;
begin
+ {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
if InterlockedDecrement(fLock)=0 then
- {$IFDEF USE_GTK_MAIN_OLD_ITERATION}
LeaveCriticalsection(FCritSec);
{$ELSE}
- g_main_context_release(FMainContext);
+ if GetCurrentThreadId = MainThreadID then
+ g_main_context_release(FMainContext)
+ else
+ LeaveCriticalsection(FCritSec)
{$ENDIF}
end;
|
|
|
Note on the Interlocked... I may have been wrong with that suggestion. It is possible than: T1: InterLockedDec reaches 0 T2: Does an InterlockInc T2: enters critical section // g_main_context_acquire(FMainContext) => but it has not yet been released T1: enters critical section // g_main_release_acquire(FMainContext) - So either the entire code must be in a critical section. - Or if a critical section is entered, the Flock value must be checked again, and rather complex handling would be needed. => Probably better to do all in a critical section. Sorry for the misleading idea. |
|
|
I still had problems in 2.0.4. Updated to Lazarus 2.0.6. Still error, with on stack view “lock” from file gtk2msgqueue.pp after postmessage . Expected to be solved in 2.0.4? Used the patch gtk2msgqueue4.patch, which still worked on the new gtk2msgqueue.pp. Found that I had to rebuild lazarus to update gtk2msgqueue.ppu (“sudo make bigide” in lazarus directory). After this patch, the problems where solved. |
|
|
LCL-GTK2 has no active maintainer who really knows the code. Thus I am applying the patch. How big speed penalty does it cause? I guess r61026 by Martin is still OK. First I though USE_GTK_MAIN_OLD_ITERATION can be removed as an old leftover but apparently it is still used for Windows. Can somebody please test if it really is needed there still. The comment about hanging is very old. {$define USE_GTK_MAIN_OLD_ITERATION} // in other case it hangs |
|
|
Please do not remove USE_GTK_MAIN_OLD_ITERATION since it is still used by users with older linux installations (eg Debian with gtk2-2.14) |
|
|
I had the problems on Linux Mint 19.2 Tina 64-bits, Mate 1.22.0. That’s the last version, so not very old. Because the problem is in a gtk part, I thought it was obvious that Linux was the OS. I had tested my program with Windows 10 (default LCLwidgettype) , but not that extensively (My windows computer was very slow). I had no problems with Windows, but that testing was much shorter. |
|
|
I applied the patch. Thanks. > Because the problem is in a gtk part, I thought it was obvious that Linux was the OS. No, it is not obvious. GTK2 works under many systems including Windows although I don't think many people use it there any more. Anyway, apparently USE_GTK_MAIN_OLD_ITERATION is still needed also with Linux. Please look at file gtk2defines.inc : {$ifdef Unix} ... {$else} {$define GTK_2_10} {$define USE_GTK_MAIN_OLD_ITERATION} // in other case it hangs {$endif} |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2019-04-16 10:04 | Boris Glavin | New Issue | |
| 2019-04-17 17:14 | Zeljan Rikalo | Note Added: 0115598 | |
| 2019-04-17 18:19 | Martin Friebe | Note Added: 0115603 | |
| 2019-04-17 18:53 | Boris Glavin | Note Added: 0115605 | |
| 2019-04-17 22:21 | Boris Glavin | File Added: gtk2msgqueue.path | |
| 2019-04-17 22:38 | Boris Glavin | Note Edited: 0115605 | View Revisions |
| 2019-04-17 22:47 | Boris Glavin | File Added: gtk2msgqueue1.path | |
| 2019-04-19 22:51 | Boris Glavin | Note Added: 0115683 | |
| 2019-04-20 12:41 | Martin Friebe | Assigned To | => Martin Friebe |
| 2019-04-20 12:41 | Martin Friebe | Status | new => assigned |
| 2019-04-20 22:26 | Martin Friebe | Status | assigned => resolved |
| 2019-04-20 22:26 | Martin Friebe | Resolution | open => fixed |
| 2019-04-20 22:26 | Martin Friebe | Fixed in Version | => 2.0.4 |
| 2019-04-20 22:26 | Martin Friebe | Fixed in Revision | => 61026 |
| 2019-04-20 22:26 | Martin Friebe | LazTarget | => 2.0.4 |
| 2019-04-20 22:26 | Martin Friebe | Widgetset | GTK 2 => GTK 2 |
| 2019-04-20 22:26 | Martin Friebe | Note Added: 0115693 | |
| 2019-04-21 22:39 | Boris Glavin | File Added: gtk2msgqueue3.path | |
| 2019-04-21 22:39 | Boris Glavin | File Added: test.zip | |
| 2019-04-21 22:39 | Boris Glavin | Note Added: 0115713 | |
| 2019-04-22 09:03 | Boris Glavin | File Added: testgtk2msgqueue.zip | |
| 2019-04-22 09:03 | Boris Glavin | File Added: gtk2msgqueue4.path | |
| 2019-04-22 09:03 | Boris Glavin | Note Added: 0115718 | |
| 2019-04-22 12:26 | Martin Friebe | Assigned To | Martin Friebe => |
| 2019-04-22 12:26 | Martin Friebe | Status | resolved => assigned |
| 2019-04-22 12:26 | Martin Friebe | Resolution | fixed => reopened |
| 2019-04-22 12:26 | Martin Friebe | LazTarget | 2.0.4 => - |
| 2019-04-22 12:26 | Martin Friebe | Note Added: 0115724 | |
| 2019-04-22 12:26 | Martin Friebe | Status | assigned => new |
| 2019-04-22 12:26 | Martin Friebe | Widgetset | GTK 2 => GTK 2 |
| 2019-04-24 20:34 | Juha Manninen | Note Added: 0115779 | |
| 2019-04-24 20:42 | Boris Glavin | File Added: gtk2msgqueue4.patch | |
| 2019-04-24 20:42 | Boris Glavin | Note Added: 0115780 | |
| 2019-04-27 22:51 | Martin Friebe | Note Added: 0115859 | |
| 2019-11-07 10:56 | Andre Kappert | Note Added: 0119130 | |
| 2019-11-09 11:08 | Juha Manninen | Assigned To | => Juha Manninen |
| 2019-11-09 11:08 | Juha Manninen | Status | new => assigned |
| 2019-11-09 11:21 | Juha Manninen | Note Added: 0119172 | |
| 2019-11-09 11:37 | Juha Manninen | Status | assigned => feedback |
| 2019-11-09 12:33 | Zeljan Rikalo | Note Added: 0119174 | |
| 2019-11-09 16:19 | Andre Kappert | Note Added: 0119175 | |
| 2019-11-10 13:14 | Juha Manninen | Status | feedback => resolved |
| 2019-11-10 13:14 | Juha Manninen | Fixed in Revision | 61026 => r61026, r62219 |
| 2019-11-10 13:14 | Juha Manninen | Widgetset | GTK 2 => GTK 2 |
| 2019-11-10 13:14 | Juha Manninen | Note Added: 0119184 | |
| 2019-11-25 17:44 | Juha Manninen | Relationship added | related to 0036359 |
| 2019-12-13 23:19 | Juha Manninen | Relationship deleted | related to 0036359 |
