View Issue Details

IDProjectCategoryView StatusLast Update
0036149FPCRTLpublic2019-11-25 21:59
ReporterBenjamin RosseauxAssigned ToFlorian 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.3.1Product BuildSVN Rev 43151 
Target VersionFixed in Version3.3.1 
Summary0036149: TEvent.WaitFor on *nix uses Realtime Clock, and not the Monotonic Clock
Description
The TEvent.WaitFor stuff at FPC is probably wrong at the moment, because it uses the Realtime Clock and not the Monotonic Clock, see https://bugs.python.org/issue23428 and https://linux.die.net/man/3/pthread_condattr_setclock and https://stackoverflow.com/questions/14248033/clock-monotonic-and-pthread-mutex-timedlock-pthread-cond-timedwait.

I suspect that we are, because we have to set monotonic-clock (if it is not default), also have to fix the freepascal code to use monotonic clock. Theoretically there should also be cond_relative_timed_wait, but I have not find much about that.

(And it is a bad design to make a timeout as absolute time in this case.)

Accordingly I have attached an appropriate patch here.
Steps To Reproduce1. Compile test0.pas without my patch, run it, set your realtime clock backwards and/or forwards in the time, and see what then happens.
2. And then compile test0.pas with my patch, run it, set your realtime clock backwards and/or forwards in the time, and see what then happens in the contrast.

TagsNo tags attached.
Fixed in Revision43589
FPCOldBugId
FPCTarget-
Attached Files
  • cthreads_monotonic_clock.patch (7,234 bytes)
    Index: rtl/linux/pthread.inc
    ===================================================================
    --- rtl/linux/pthread.inc	(revision 43151)
    +++ rtl/linux/pthread.inc	(working copy)
    @@ -153,6 +153,7 @@
         function pthread_cond_timedwait(__cond:ppthread_cond_t; __mutex:ppthread_mutex_t; __abstime:ptimespec):longint;cdecl;external;
         function pthread_condattr_init(__attr:ppthread_condattr_t):longint;cdecl;external;
         function pthread_condattr_destroy(__attr:ppthread_condattr_t):longint;cdecl;external;
    +    function pthread_condattr_setclock(__attr:ppthread_condattr_t; __clock_id: longint):longint;cdecl;external;
         function pthread_key_create(__key:ppthread_key_t; __destr_function:__destr_function_t):longint;cdecl;external;
         function pthread_key_delete(__key:pthread_key_t):longint;cdecl;external;
         function pthread_setspecific(__key:pthread_key_t; __pointer:pointer):longint;cdecl;external;
    @@ -228,6 +229,7 @@
     {$ifndef ANDROID}
         pthread_condattr_init : Function(__attr:ppthread_condattr_t):longint;cdecl;
         pthread_condattr_destroy : Function(__attr:ppthread_condattr_t):longint;cdecl;
    +    pthread_condattr_setclock: Function(__attr:ppthread_condattr_t; __clock_id: longint):longint;cdecl;
     {$endif}
         pthread_key_create : Function(__key:ppthread_key_t; __destr_function:__destr_function_t):longint;cdecl;
         pthread_key_delete : Function(__key:pthread_key_t):longint;cdecl;
    @@ -321,6 +323,7 @@
     {$ifndef ANDROID}
       Pointer(pthread_condattr_init) := dlsym(PthreadDLL,'pthread_condattr_init');
       Pointer(pthread_condattr_destroy) := dlsym(PthreadDLL,'pthread_condattr_destroy');
    +  Pointer(pthread_condattr_setclock) := dlsym(PthreadDLL,'pthread_condattr_setclock');
     {$endif}
       Pointer(pthread_key_create) := dlsym(PthreadDLL,'pthread_key_create');
       Pointer(pthread_key_delete) := dlsym(PthreadDLL,'pthread_key_delete');
    Index: rtl/unix/cthreads.pp
    ===================================================================
    --- rtl/unix/cthreads.pp	(revision 43151)
    +++ rtl/unix/cthreads.pp	(working copy)
    @@ -67,6 +67,9 @@
     implementation
     
     Uses
    +{$if defined(Linux) and not defined(Android)}
    +  Linux,
    +{$endif}
       BaseUnix,
       unix,
       unixtype,
    @@ -553,6 +556,10 @@
          TPthreadMutex = pthread_mutex_t;
          Tbasiceventstate=record
              FCondVar: TPthreadCondition;
    +{$if defined(Linux) and not defined(Android)}         
    +         FAttr: pthread_condattr_t;
    +         FClockID: longint;
    +{$ifend}        
              FEventSection: TPthreadMutex;
              FWaiters: longint;
              FIsSet,
    @@ -571,7 +578,10 @@
     function IntBasicEventCreate(EventAttributes : Pointer; AManualReset,InitialState : Boolean;const Name : ansistring):pEventState;
     var
       MAttr : pthread_mutexattr_t;
    -  res   : cint;
    +  res   : cint;  
    +{$if defined(Linux) and not defined(Android)}  
    +  timespec: ttimespec;
    +{$ifend}  
     begin
       new(plocaleventstate(result));
       plocaleventstate(result)^.FManualReset:=AManualReset;
    @@ -578,12 +588,57 @@
       plocaleventstate(result)^.FWaiters:=0;
       plocaleventstate(result)^.FDestroying:=False;
       plocaleventstate(result)^.FIsSet:=InitialState;
    +{$if defined(Linux) and not defined(Android)}  
    +  res := pthread_condattr_init(@plocaleventstate(result)^.FAttr);
    +  if (res <> 0) then
    +  begin
    +    FreeMem(result);
    +    fpc_threaderror;  
    +  end;
    +  
    +  if clock_gettime(CLOCK_MONOTONIC_RAW, @timespec) = 0 then
    +  begin
    +    res := pthread_condattr_setclock(@plocaleventstate(result)^.FAttr, CLOCK_MONOTONIC_RAW);
    +  end
    +  else
    +  begin
    +    res := -1; // No support for CLOCK_MONOTONIC_RAW   
    +  end;
    +  
    +  if (res = 0) then
    +  begin
    +    plocaleventstate(result)^.FClockID := CLOCK_MONOTONIC_RAW;
    +  end
    +  else
    +  begin
    +    res := pthread_condattr_setclock(@plocaleventstate(result)^.FAttr, CLOCK_MONOTONIC);
    +    if (res = 0) then
    +    begin
    +      plocaleventstate(result)^.FClockID := CLOCK_MONOTONIC;
    +    end
    +    else
    +    begin
    +      pthread_condattr_destroy(@plocaleventstate(result)^.FAttr);
    +      FreeMem(result);
    +      fpc_threaderror;  
    +    end;    
    +  end;  
    +
    +  res := pthread_cond_init(@plocaleventstate(result)^.FCondVar, @plocaleventstate(result)^.FAttr);
    +  if (res <> 0) then
    +  begin
    +    pthread_condattr_destroy(@plocaleventstate(result)^.FAttr);  
    +    FreeMem(result);
    +    fpc_threaderror;
    +  end;
    +{$else}
       res := pthread_cond_init(@plocaleventstate(result)^.FCondVar, nil);
       if (res <> 0) then
       begin
         FreeMem(result);
         fpc_threaderror;
    -  end;
    +  end; 
    +{$ifend} 
     
       res:=pthread_mutexattr_init(@MAttr);
       if res=0 then
    @@ -601,6 +656,9 @@
       if res <> 0 then
         begin
           pthread_cond_destroy(@plocaleventstate(result)^.FCondVar);
    +{$if defined(Linux) and not defined(Android)}  
    +      pthread_condattr_destroy(@plocaleventstate(result)^.FAttr);	
    +{$ifend}      
           FreeMem(result);
           fpc_threaderror;
         end;
    @@ -622,6 +680,9 @@
     
       { and clean up }
       pthread_cond_destroy(@plocaleventstate(state)^.Fcondvar);
    +{$if defined(Linux) and not defined(Android)}  
    +  pthread_condattr_destroy(@plocaleventstate(state)^.FAttr);	
    +{$ifend}  
       pthread_mutex_destroy(@plocaleventstate(state)^.FEventSection);
       dispose(plocaleventstate(state));
     end;
    @@ -652,6 +713,7 @@
       isset: boolean;
       tnow : timeval;
     begin
    +
       { safely check whether we are being destroyed, if so immediately return. }
       { otherwise (under the same mutex) increase the number of waiters        }
       pthread_mutex_lock(@plocaleventstate(state)^.feventsection);
    @@ -674,17 +736,32 @@
       else
         begin
           //Wait with timeout using pthread_cond_timedwait
    -      fpgettimeofday(@tnow,nil);
    +{$if defined(Linux) and not defined(Android)}
    +      if clock_gettime(plocaleventstate(state)^.FClockID, @timespec) <> 0 then
    +      begin
    +        Result := Ord(wrError);
    +        Exit;
    +      end;
    +      timespec.tv_sec  := timespec.tv_sec + (clong(timeout) div 1000);
    +      timespec.tv_nsec := ((clong(timeout) mod 1000) * 1000000) + (timespec.tv_nsec);
    +{$else}
    +      // TODO: FIX-ME: Also use monotonic clock for other *nix targets
    +      fpgettimeofday(@tnow, nil);
           timespec.tv_sec  := tnow.tv_sec + (clong(timeout) div 1000);
    -      timespec.tv_nsec := (clong(timeout) mod 1000)*1000000 + tnow.tv_usec*1000;
    +      timespec.tv_nsec := ((clong(timeout) mod 1000) * 1000000) + (tnow.tv_usec * 1000);
    +{$ifend}
           if timespec.tv_nsec >= 1000000000 then
             begin
               inc(timespec.tv_sec);
               dec(timespec.tv_nsec, 1000000000);
             end;
    -      errres:=0;
    -      while (not plocaleventstate(state)^.FDestroying) and (not plocaleventstate(state)^.FIsSet) and (errres<>ESysETIMEDOUT) do
    -        errres:=pthread_cond_timedwait(@plocaleventstate(state)^.Fcondvar, @plocaleventstate(state)^.feventsection, @timespec);
    +      errres := 0;
    +      while (not plocaleventstate(state)^.FDestroying) and
    +            (not plocaleventstate(state)^.FIsSet) and 
    +            (errres<>ESysETIMEDOUT) do
    +        errres := pthread_cond_timedwait(@plocaleventstate(state)^.Fcondvar,
    +                                         @plocaleventstate(state)^.feventsection, 
    +                                         @timespec);
         end;
     
       isset := plocaleventstate(state)^.FIsSet;
    
  • test0.pas (257 bytes)
    program test0;
    {$mode delphi}
    uses cthreads,SysUtils,Classes,SyncObjs; //,betterevent;
    var e:TEvent;
    begin
     writeln('A');
     e:=TEvent.Create(nil,false,false,'');
     try
      e.WaitFor(60000);
     finally
      FreeAndNil(e); 
     end; 
     writeln('B');
    end. 
    
    
    test0.pas (257 bytes)

Activities

Benjamin Rosseaux

2019-10-08 13:59

reporter  

cthreads_monotonic_clock.patch (7,234 bytes)
Index: rtl/linux/pthread.inc
===================================================================
--- rtl/linux/pthread.inc	(revision 43151)
+++ rtl/linux/pthread.inc	(working copy)
@@ -153,6 +153,7 @@
     function pthread_cond_timedwait(__cond:ppthread_cond_t; __mutex:ppthread_mutex_t; __abstime:ptimespec):longint;cdecl;external;
     function pthread_condattr_init(__attr:ppthread_condattr_t):longint;cdecl;external;
     function pthread_condattr_destroy(__attr:ppthread_condattr_t):longint;cdecl;external;
+    function pthread_condattr_setclock(__attr:ppthread_condattr_t; __clock_id: longint):longint;cdecl;external;
     function pthread_key_create(__key:ppthread_key_t; __destr_function:__destr_function_t):longint;cdecl;external;
     function pthread_key_delete(__key:pthread_key_t):longint;cdecl;external;
     function pthread_setspecific(__key:pthread_key_t; __pointer:pointer):longint;cdecl;external;
@@ -228,6 +229,7 @@
 {$ifndef ANDROID}
     pthread_condattr_init : Function(__attr:ppthread_condattr_t):longint;cdecl;
     pthread_condattr_destroy : Function(__attr:ppthread_condattr_t):longint;cdecl;
+    pthread_condattr_setclock: Function(__attr:ppthread_condattr_t; __clock_id: longint):longint;cdecl;
 {$endif}
     pthread_key_create : Function(__key:ppthread_key_t; __destr_function:__destr_function_t):longint;cdecl;
     pthread_key_delete : Function(__key:pthread_key_t):longint;cdecl;
@@ -321,6 +323,7 @@
 {$ifndef ANDROID}
   Pointer(pthread_condattr_init) := dlsym(PthreadDLL,'pthread_condattr_init');
   Pointer(pthread_condattr_destroy) := dlsym(PthreadDLL,'pthread_condattr_destroy');
+  Pointer(pthread_condattr_setclock) := dlsym(PthreadDLL,'pthread_condattr_setclock');
 {$endif}
   Pointer(pthread_key_create) := dlsym(PthreadDLL,'pthread_key_create');
   Pointer(pthread_key_delete) := dlsym(PthreadDLL,'pthread_key_delete');
Index: rtl/unix/cthreads.pp
===================================================================
--- rtl/unix/cthreads.pp	(revision 43151)
+++ rtl/unix/cthreads.pp	(working copy)
@@ -67,6 +67,9 @@
 implementation
 
 Uses
+{$if defined(Linux) and not defined(Android)}
+  Linux,
+{$endif}
   BaseUnix,
   unix,
   unixtype,
@@ -553,6 +556,10 @@
      TPthreadMutex = pthread_mutex_t;
      Tbasiceventstate=record
          FCondVar: TPthreadCondition;
+{$if defined(Linux) and not defined(Android)}         
+         FAttr: pthread_condattr_t;
+         FClockID: longint;
+{$ifend}        
          FEventSection: TPthreadMutex;
          FWaiters: longint;
          FIsSet,
@@ -571,7 +578,10 @@
 function IntBasicEventCreate(EventAttributes : Pointer; AManualReset,InitialState : Boolean;const Name : ansistring):pEventState;
 var
   MAttr : pthread_mutexattr_t;
-  res   : cint;
+  res   : cint;  
+{$if defined(Linux) and not defined(Android)}  
+  timespec: ttimespec;
+{$ifend}  
 begin
   new(plocaleventstate(result));
   plocaleventstate(result)^.FManualReset:=AManualReset;
@@ -578,12 +588,57 @@
   plocaleventstate(result)^.FWaiters:=0;
   plocaleventstate(result)^.FDestroying:=False;
   plocaleventstate(result)^.FIsSet:=InitialState;
+{$if defined(Linux) and not defined(Android)}  
+  res := pthread_condattr_init(@plocaleventstate(result)^.FAttr);
+  if (res <> 0) then
+  begin
+    FreeMem(result);
+    fpc_threaderror;  
+  end;
+  
+  if clock_gettime(CLOCK_MONOTONIC_RAW, @timespec) = 0 then
+  begin
+    res := pthread_condattr_setclock(@plocaleventstate(result)^.FAttr, CLOCK_MONOTONIC_RAW);
+  end
+  else
+  begin
+    res := -1; // No support for CLOCK_MONOTONIC_RAW   
+  end;
+  
+  if (res = 0) then
+  begin
+    plocaleventstate(result)^.FClockID := CLOCK_MONOTONIC_RAW;
+  end
+  else
+  begin
+    res := pthread_condattr_setclock(@plocaleventstate(result)^.FAttr, CLOCK_MONOTONIC);
+    if (res = 0) then
+    begin
+      plocaleventstate(result)^.FClockID := CLOCK_MONOTONIC;
+    end
+    else
+    begin
+      pthread_condattr_destroy(@plocaleventstate(result)^.FAttr);
+      FreeMem(result);
+      fpc_threaderror;  
+    end;    
+  end;  
+
+  res := pthread_cond_init(@plocaleventstate(result)^.FCondVar, @plocaleventstate(result)^.FAttr);
+  if (res <> 0) then
+  begin
+    pthread_condattr_destroy(@plocaleventstate(result)^.FAttr);  
+    FreeMem(result);
+    fpc_threaderror;
+  end;
+{$else}
   res := pthread_cond_init(@plocaleventstate(result)^.FCondVar, nil);
   if (res <> 0) then
   begin
     FreeMem(result);
     fpc_threaderror;
-  end;
+  end; 
+{$ifend} 
 
   res:=pthread_mutexattr_init(@MAttr);
   if res=0 then
@@ -601,6 +656,9 @@
   if res <> 0 then
     begin
       pthread_cond_destroy(@plocaleventstate(result)^.FCondVar);
+{$if defined(Linux) and not defined(Android)}  
+      pthread_condattr_destroy(@plocaleventstate(result)^.FAttr);	
+{$ifend}      
       FreeMem(result);
       fpc_threaderror;
     end;
@@ -622,6 +680,9 @@
 
   { and clean up }
   pthread_cond_destroy(@plocaleventstate(state)^.Fcondvar);
+{$if defined(Linux) and not defined(Android)}  
+  pthread_condattr_destroy(@plocaleventstate(state)^.FAttr);	
+{$ifend}  
   pthread_mutex_destroy(@plocaleventstate(state)^.FEventSection);
   dispose(plocaleventstate(state));
 end;
@@ -652,6 +713,7 @@
   isset: boolean;
   tnow : timeval;
 begin
+
   { safely check whether we are being destroyed, if so immediately return. }
   { otherwise (under the same mutex) increase the number of waiters        }
   pthread_mutex_lock(@plocaleventstate(state)^.feventsection);
@@ -674,17 +736,32 @@
   else
     begin
       //Wait with timeout using pthread_cond_timedwait
-      fpgettimeofday(@tnow,nil);
+{$if defined(Linux) and not defined(Android)}
+      if clock_gettime(plocaleventstate(state)^.FClockID, @timespec) <> 0 then
+      begin
+        Result := Ord(wrError);
+        Exit;
+      end;
+      timespec.tv_sec  := timespec.tv_sec + (clong(timeout) div 1000);
+      timespec.tv_nsec := ((clong(timeout) mod 1000) * 1000000) + (timespec.tv_nsec);
+{$else}
+      // TODO: FIX-ME: Also use monotonic clock for other *nix targets
+      fpgettimeofday(@tnow, nil);
       timespec.tv_sec  := tnow.tv_sec + (clong(timeout) div 1000);
-      timespec.tv_nsec := (clong(timeout) mod 1000)*1000000 + tnow.tv_usec*1000;
+      timespec.tv_nsec := ((clong(timeout) mod 1000) * 1000000) + (tnow.tv_usec * 1000);
+{$ifend}
       if timespec.tv_nsec >= 1000000000 then
         begin
           inc(timespec.tv_sec);
           dec(timespec.tv_nsec, 1000000000);
         end;
-      errres:=0;
-      while (not plocaleventstate(state)^.FDestroying) and (not plocaleventstate(state)^.FIsSet) and (errres<>ESysETIMEDOUT) do
-        errres:=pthread_cond_timedwait(@plocaleventstate(state)^.Fcondvar, @plocaleventstate(state)^.feventsection, @timespec);
+      errres := 0;
+      while (not plocaleventstate(state)^.FDestroying) and
+            (not plocaleventstate(state)^.FIsSet) and 
+            (errres<>ESysETIMEDOUT) do
+        errres := pthread_cond_timedwait(@plocaleventstate(state)^.Fcondvar,
+                                         @plocaleventstate(state)^.feventsection, 
+                                         @timespec);
     end;
 
   isset := plocaleventstate(state)^.FIsSet;
test0.pas (257 bytes)
program test0;
{$mode delphi}
uses cthreads,SysUtils,Classes,SyncObjs; //,betterevent;
var e:TEvent;
begin
 writeln('A');
 e:=TEvent.Create(nil,false,false,'');
 try
  e.WaitFor(60000);
 finally
  FreeAndNil(e); 
 end; 
 writeln('B');
end. 

test0.pas (257 bytes)

Benjamin Rosseaux

2019-10-08 14:01

reporter   ~0118415

And the patch patches the problem only for Linux at the moment, on the other targets the problem persists.

Benjamin Rosseaux

2019-10-14 13:14

reporter   ~0118584

As a supplement: This issue is very evil because if your program waits on TEvent and something or someone changes the clock, for example if NTP does that, it will get completely stuck, so this is more of a fatal bug than just a "minor" issue.

Thaddy de Koning

2019-10-14 15:50

reporter   ~0118589

Yes, confirmed on one of my Raspbian servers (manually, not NTP).
Specifically in server environments current behavior can be lethal.
I support your suggestion to raise the urgency.

Florian

2019-11-25 21:59

administrator   ~0119493

Thank you, applied.

Issue History

Date Modified Username Field Change
2019-10-08 13:59 Benjamin Rosseaux New Issue
2019-10-08 13:59 Benjamin Rosseaux File Added: cthreads_monotonic_clock.patch
2019-10-08 13:59 Benjamin Rosseaux File Added: test0.pas
2019-10-08 14:01 Benjamin Rosseaux Note Added: 0118415
2019-10-14 13:14 Benjamin Rosseaux Note Added: 0118584
2019-10-14 15:50 Thaddy de Koning Note Added: 0118589
2019-11-25 21:59 Florian Assigned To => Florian
2019-11-25 21:59 Florian Status new => resolved
2019-11-25 21:59 Florian Resolution open => fixed
2019-11-25 21:59 Florian Fixed in Version => 3.3.1
2019-11-25 21:59 Florian Fixed in Revision => 43589
2019-11-25 21:59 Florian FPCTarget => -
2019-11-25 21:59 Florian Note Added: 0119493