View Issue Details

IDProjectCategoryView StatusLast Update
0035210LazarusTAChartpublic2019-03-31 16:15
ReporterMarcin WiazowskiAssigned Towp 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version2.1 (SVN)Product Build60628 
Target Version2.2Fixed in Version 
Summary0035210: TAChart: two problems with notification broadcasting
DescriptionWhen data source is changed, it notifies all its clients of this. But there are two problems with the notification code:

1) Client can remove itself from the notification list just at the moment when it's being notified - this changes list's contents, but is not taken into account by the notification procedure. This leads to omissions when notifying.

2) Client can raise an exception just at the moment when it's being notified - which breaks the notification procedure prematurely. This leads to omissions when notifying.



The attached Reproduce application has three charts, each with one line series. All series have same source (ListChartSource). Pressing a "Test" button changes ListChartSource's YCount to zero, thus making the source improper for line series. In this case, first notified series reverts to its built-in source (so it disappears from the chart) and raises an exception - which breaks the notification chain. As a consequence, the other two series still use (already invalid) source. Pressing the "Test" button again repeats this behavior, making the next series reverting to its built-in source, disappearing from the chart and raising an exception. The third attempt removes the last series from the chart.

The problem can be also reproduced in IDE, when changing ListChartSource's YCount to 0 and then back to 1, and again, and again.



The attached patch solves both 1) and 2) problems. It catches all the exceptions raised during the notification process, and then raises a one, common exception. Thanks to using a "dupIgnore" option, same exception messages are reported only once.

After applying the patch and pressing the "Test" button, all three series disappear at the same moment from their charts.
TagsNo tags attached.
Fixed in Revision60657, 60801
LazTarget2.2
WidgetsetWin32/Win64
Attached Files
  • Reproduce.zip (2,451 bytes)
  • patch.diff (1,640 bytes)
    Index: components/tachart/tachartutils.pas
    ===================================================================
    --- components/tachart/tachartutils.pas	(revision 60628)
    +++ components/tachart/tachartutils.pas	(working copy)
    @@ -43,6 +43,7 @@
     type
       EChartError = class(Exception);
       EChartIntervalError = class(EChartError);
    +  EBroadcasterError = class(EChartError);
       EListenerError = class(EChartError);
       EDrawDataError = class(EChartError);
     
    @@ -847,11 +848,40 @@
     
     procedure TBroadcaster.Broadcast(ASender: TObject);
     var
    -  p: Pointer;
    +  ListCopy: array of Pointer;
    +  Exceptions: TStringList;
    +  i: Integer;
     begin
       if Locked then exit;
    -  for p in Self do
    -    TListener(p).Notify(ASender);
    +  if Count = 0 then exit;
    +
    +  // Listeners can remove themselves when being notified, which
    +  // changes the list - so we must use a copy of the list when
    +  // notifying, to avoid omissions in notifying
    +  SetLength(ListCopy, Count);
    +  for i := 0 to High(ListCopy) do
    +    ListCopy[i] := List^[i];
    +
    +  Exceptions := nil;
    +  try
    +    for i := 0 to High(ListCopy) do
    +    try
    +      TListener(ListCopy[i]).Notify(ASender);
    +    except
    +      on E: Exception do begin
    +        if not Assigned(Exceptions) then begin
    +          Exceptions := TStringList.Create;
    +          Exceptions.Duplicates := dupIgnore;
    +          Exceptions.Sorted := true; // required by dupIgnore
    +        end;
    +        Exceptions.Add(E.Message);
    +      end;
    +    end;
    +    if Assigned(Exceptions) then
    +      raise EBroadcasterError.Create(Trim(Exceptions.Text));
    +  finally
    +    Exceptions.Free;
    +  end;
     end;
     
     destructor TBroadcaster.Destroy;
    
    patch.diff (1,640 bytes)
  • Reproduce2.zip (2,469 bytes)
  • Image2a.png (26,513 bytes)
    Image2a.png (26,513 bytes)
  • Image2b.png (8,967 bytes)
    Image2b.png (8,967 bytes)
  • Image2c.png (13,819 bytes)
    Image2c.png (13,819 bytes)
  • update.diff (1,435 bytes)
    Index: components/tachart/tachartutils.pas
    ===================================================================
    --- components/tachart/tachartutils.pas	(revision 60792)
    +++ components/tachart/tachartutils.pas	(working copy)
    @@ -847,6 +847,7 @@
     var
       ListCopy: array of Pointer;
       Exceptions: TStringList;
    +  Aborted: Boolean;
       i: Integer;
     begin
       if Locked then exit;
    @@ -860,22 +861,28 @@
         ListCopy[i] := List^[i];
     
       Exceptions := nil;
    +  Aborted := False;
       try
         for i := 0 to High(ListCopy) do
         try
           TListener(ListCopy[i]).Notify(ASender);
         except
    -      on E: Exception do begin
    -        if not Assigned(Exceptions) then begin
    -          Exceptions := TStringList.Create;
    -          Exceptions.Duplicates := dupIgnore;
    -          Exceptions.Sorted := true; // required by dupIgnore
    +      on E: Exception do
    +        if E is EAbort then
    +          Aborted := true
    +        else begin
    +          if not Assigned(Exceptions) then begin
    +            Exceptions := TStringList.Create;
    +            Exceptions.Duplicates := dupIgnore;
    +            Exceptions.Sorted := true; // required by dupIgnore
    +          end;
    +          Exceptions.Add(E.Message);
             end;
    -        Exceptions.Add(E.Message);
    -      end;
         end;
         if Assigned(Exceptions) then
           raise EBroadcasterError.Create(Trim(Exceptions.Text));
    +    if Aborted then
    +      Abort;
       finally
         Exceptions.Free;
       end;
    
    update.diff (1,435 bytes)

Activities

Marcin Wiazowski

2019-03-09 01:21

reporter  

Reproduce.zip (2,451 bytes)

Marcin Wiazowski

2019-03-09 01:21

reporter  

patch.diff (1,640 bytes)
Index: components/tachart/tachartutils.pas
===================================================================
--- components/tachart/tachartutils.pas	(revision 60628)
+++ components/tachart/tachartutils.pas	(working copy)
@@ -43,6 +43,7 @@
 type
   EChartError = class(Exception);
   EChartIntervalError = class(EChartError);
+  EBroadcasterError = class(EChartError);
   EListenerError = class(EChartError);
   EDrawDataError = class(EChartError);
 
@@ -847,11 +848,40 @@
 
 procedure TBroadcaster.Broadcast(ASender: TObject);
 var
-  p: Pointer;
+  ListCopy: array of Pointer;
+  Exceptions: TStringList;
+  i: Integer;
 begin
   if Locked then exit;
-  for p in Self do
-    TListener(p).Notify(ASender);
+  if Count = 0 then exit;
+
+  // Listeners can remove themselves when being notified, which
+  // changes the list - so we must use a copy of the list when
+  // notifying, to avoid omissions in notifying
+  SetLength(ListCopy, Count);
+  for i := 0 to High(ListCopy) do
+    ListCopy[i] := List^[i];
+
+  Exceptions := nil;
+  try
+    for i := 0 to High(ListCopy) do
+    try
+      TListener(ListCopy[i]).Notify(ASender);
+    except
+      on E: Exception do begin
+        if not Assigned(Exceptions) then begin
+          Exceptions := TStringList.Create;
+          Exceptions.Duplicates := dupIgnore;
+          Exceptions.Sorted := true; // required by dupIgnore
+        end;
+        Exceptions.Add(E.Message);
+      end;
+    end;
+    if Assigned(Exceptions) then
+      raise EBroadcasterError.Create(Trim(Exceptions.Text));
+  finally
+    Exceptions.Free;
+  end;
 end;
 
 destructor TBroadcaster.Destroy;
patch.diff (1,640 bytes)

wp

2019-03-10 11:27

developer   ~0114763

> Client can remove itself from the notification list just at the moment when it's
> being notified

Notification broadcasting is an internal process of TAChart which should not be modified by user code. If this is done nevertheless I do not have enough imagination about what the correct behavior should be. Maybe the programmer has good reason to remove an element from the notification list. With your patch, this is much harder or even impossible. To me there is no reason to think why the current behavior is wrong.

--------------

> Client can raise an exception just at the moment when it's being notified -
> which breaks the notification procedure prematurely. This leads to omissions
> when notifying.

Yes. But isn't this the way exceptions should work? They interrupt the current operation and leave the rest of the calling stack alone. Your patch modifies this behavior and makes sure that the rest of the notification chain is executed. Again, I cannot evaluate whether this is correct or wrong. The result is wrong in this particular case in any way: No function is displayed at all - this is certainly not what the programmer wanted to achieve when he stupidly set XCount to 0. Is simply is a programming error, and in this case the library should raise an exception with an appropriate message text and stop the action. But that's what the current version does. So why the extension? Because the result looks more consistent? Why should it? It is wrong in any way.

Considering your example I find again that setting XCount and YCount to zero leads to problems. In my eyes, having XCount and YCount > 0 is one of the key assumptions of the TAChart, probably of any plotting library. I regret having allowed XCount=0 and YCount=0 for the DataPoints editor, it opens the mind of the user that this use case might be useful for anything else than just axis labeling. I have not yet come to a decision, but I tend to raise an exception when XCount or YCount are set to a value < 1.

Marcin Wiazowski

2019-03-10 21:37

reporter  

Reproduce2.zip (2,469 bytes)

Marcin Wiazowski

2019-03-10 21:38

reporter  

Image2a.png (26,513 bytes)
Image2a.png (26,513 bytes)

Marcin Wiazowski

2019-03-10 21:38

reporter  

Image2b.png (8,967 bytes)
Image2b.png (8,967 bytes)

Marcin Wiazowski

2019-03-10 21:38

reporter  

Image2c.png (13,819 bytes)
Image2c.png (13,819 bytes)

Marcin Wiazowski

2019-03-10 21:48

reporter   ~0114780

Well, my sample application was unfortunate, because it suggested, that the problem is in having XCount or YCount = 0 - although the problem is elsewhere. I also must admit, that I wasn't clear enough with my explanations.

So I'm attaching a Reproduce2 application here. This time, it uses TFieldSeries, with data source having XCount = YCount = 2. There is also a change in "Test" button handler - it changes YCount from 2 to 1, but does not revert it back to 2. Testing scenario is as follows:

Test 1:
1) Launch the Reproduce2 application WITHOUT THE DEBUGGER
2) Press the "Test" button - ListChartSource.YCount will be set to 1, so an exception will be raised: "TFieldSeries requires a chart source with at least 2 y value(s) per data point" (see Image2a.png)
3) As can be seen, Chart3's series disappeared, because it reverted to its built-in source (because external ListChartSource is no longer valid for TFieldSeries)
4) As can be seen, notification chain has been broken due to the exception, so Chart1 and Chart2 have NOT been notified about ListChartSource change - so they still use the ListChartSource, although its no longer valid for TFieldSeries
5) Minimize the application and restore it again, so charts will be repainted: most probably, the application will hang due to improper memory accesses

Test 2:
1) Launch the Reproduce2 application UNDER THE DEBUGGER
2) Press the "Test" button - ListChartSource.YCount will be set to 1, so the debugger will stop with an exception: "TFieldSeries requires a chart source with at least 2 y value(s) per data point"
3) Continue execution: most probably, you will immediately see the next exception "Project Reproduce2 raised exception class 'External: SIGSEGV'", as shown at Image2b.png - this is due to improper memory accesses in Chart1 and/or Chart2 (notification chain has been broken, so Chart1 and Chart2 haven't been notified, that ListChartSource is no longer valid - so they still use it, making out-of-bounds memory accesses)

Test 3:
1) Load the Reproduce2 application in IDE
2) In Object Inspector, change ListChartSource.YCount from 2 to 1
3) An exception will be raised: "TFieldSeries requires a chart source with at least 2 y value(s) per data point"
4) Chart3's series will revert to its built-in source, but - because notification chain has been broken - Chart1 and Chart2 will still use ListChartSource (which is no longer valid for TFieldSeries)
5) Close the message, minimize and restore the designed form, so charts will be repainted
6) Chart1 and Chart2 are now not drawn due to invalid memory accesses (see Image2c.png)
7) Lazarus IDE hangs forever or, sometimes, raises some invalid memory access exception

So this is basically the same problem as described in 0035089. The patch from 0035089 solved the problem for cases, when there is only one source's client - and the patch attached here solves just all the other cases.




About removing a client from the notification list, just at the moment when the client is being notified: I wasn't clear enough. This isn't of course something, that end-user's code is expected to do. But this is exactly what was (implicitly, unexpectedly) introduced in the 0035089 fix.

Let's analyze Reproduce2's behavior (assuming, that notification loop is not broken): notification list is as follows:

[0]: Chart3FieldSeries
[1]: Chart2FieldSeries
[2]: Chart1FieldSeries

In the broadcaster, there is a loop, which effectively works as:

i := 0;
while i < NotificationList.Count do
begin
  Line A: send notification to NotificationList[i];
  Line B: Inc(i);
end;

For i = 0, at Line A in the code above, Chart3FieldSeries is being notified. It reverts back to its built-in source - so it adds itself to built-in source's notification list, and REMOVES itself form ListChartSource source's notification list. As a consequence, our notification list becomes:

[0]: Chart2FieldSeries
[1]: Chart1FieldSeries

At Line B in the code, "i" is incremented from 0 to 1, so the next notified client will be Chart1FieldSeries. This way, we omitted Chart2FieldSeries...

The solution is just to make a copy of the list, and use this copy in the notification loop.




About breaking the notification chain: of course, when an exception is raised, it is clearly to avoid using a normal execution path. The question is: when should we revert to the normal execution path? Currently, we go a bridge too far - we break not only the notified component's execution path, but also other, unrelated components' execution paths. When Chart3 wishes to abort its execution path, it's ok - but it should not break execution path of other components, like Chart1 and Chart2.




So, basically, the patch attached here is only to avoid invalid memory accesses - and thus hangs and crashes - in the designed application and in IDE. In fact, it is just a completion of the #0035089 patch.

And well, maybe setting sources's YCount to 1 is not very clever, when the source is used by TFieldSeries - but please note, that TAChart is a very complicated component, so everyone may perform various stupid actions at the beginning...

wp

2019-03-12 10:01

developer   ~0114797

Applied, thanks. The observation that the reported behavior can block the IDE made the decision easier.

Marcin Wiazowski

2019-03-16 01:56

reporter   ~0114850

Fix confirmed. Thanks!

Marcin Wiazowski

2019-03-30 00:07

reporter   ~0115115

I'm reopening to fix an issue, that I introduced in TBroadcaster.Broadcast(): EAbort should be handled silently, but currently it isn't.

The attached update.diff fixes the issue.

Marcin Wiazowski

2019-03-30 00:07

reporter  

update.diff (1,435 bytes)
Index: components/tachart/tachartutils.pas
===================================================================
--- components/tachart/tachartutils.pas	(revision 60792)
+++ components/tachart/tachartutils.pas	(working copy)
@@ -847,6 +847,7 @@
 var
   ListCopy: array of Pointer;
   Exceptions: TStringList;
+  Aborted: Boolean;
   i: Integer;
 begin
   if Locked then exit;
@@ -860,22 +861,28 @@
     ListCopy[i] := List^[i];
 
   Exceptions := nil;
+  Aborted := False;
   try
     for i := 0 to High(ListCopy) do
     try
       TListener(ListCopy[i]).Notify(ASender);
     except
-      on E: Exception do begin
-        if not Assigned(Exceptions) then begin
-          Exceptions := TStringList.Create;
-          Exceptions.Duplicates := dupIgnore;
-          Exceptions.Sorted := true; // required by dupIgnore
+      on E: Exception do
+        if E is EAbort then
+          Aborted := true
+        else begin
+          if not Assigned(Exceptions) then begin
+            Exceptions := TStringList.Create;
+            Exceptions.Duplicates := dupIgnore;
+            Exceptions.Sorted := true; // required by dupIgnore
+          end;
+          Exceptions.Add(E.Message);
         end;
-        Exceptions.Add(E.Message);
-      end;
     end;
     if Assigned(Exceptions) then
       raise EBroadcasterError.Create(Trim(Exceptions.Text));
+    if Aborted then
+      Abort;
   finally
     Exceptions.Free;
   end;
update.diff (1,435 bytes)

wp

2019-03-31 12:45

developer   ~0115142

Applied, thanks.

Marcin Wiazowski

2019-03-31 16:15

reporter   ~0115144

Thanks!

Issue History

Date Modified Username Field Change
2019-03-09 01:21 Marcin Wiazowski New Issue
2019-03-09 01:21 Marcin Wiazowski File Added: Reproduce.zip
2019-03-09 01:21 Marcin Wiazowski File Added: patch.diff
2019-03-09 12:33 wp Assigned To => wp
2019-03-09 12:33 wp Status new => assigned
2019-03-10 11:27 wp Note Added: 0114763
2019-03-10 11:27 wp LazTarget => -
2019-03-10 11:27 wp Status assigned => resolved
2019-03-10 11:27 wp Resolution open => won't fix
2019-03-10 21:37 Marcin Wiazowski File Added: Reproduce2.zip
2019-03-10 21:38 Marcin Wiazowski File Added: Image2a.png
2019-03-10 21:38 Marcin Wiazowski File Added: Image2b.png
2019-03-10 21:38 Marcin Wiazowski File Added: Image2c.png
2019-03-10 21:48 Marcin Wiazowski Note Added: 0114780
2019-03-10 21:48 Marcin Wiazowski Status resolved => assigned
2019-03-10 21:48 Marcin Wiazowski Resolution won't fix => reopened
2019-03-12 10:01 wp Fixed in Revision => 60657
2019-03-12 10:01 wp LazTarget - => 2.2
2019-03-12 10:01 wp Note Added: 0114797
2019-03-12 10:01 wp Status assigned => resolved
2019-03-12 10:01 wp Resolution reopened => fixed
2019-03-12 10:01 wp Target Version => 2.2
2019-03-16 01:56 Marcin Wiazowski Note Added: 0114850
2019-03-16 01:56 Marcin Wiazowski Status resolved => closed
2019-03-30 00:07 Marcin Wiazowski Note Added: 0115115
2019-03-30 00:07 Marcin Wiazowski Status closed => assigned
2019-03-30 00:07 Marcin Wiazowski Resolution fixed => reopened
2019-03-30 00:07 Marcin Wiazowski File Added: update.diff
2019-03-31 12:45 wp Fixed in Revision 60657 => 60657, 60801
2019-03-31 12:45 wp Note Added: 0115142
2019-03-31 12:45 wp Status assigned => resolved
2019-03-31 12:45 wp Resolution reopened => fixed
2019-03-31 16:15 Marcin Wiazowski Note Added: 0115144
2019-03-31 16:15 Marcin Wiazowski Status resolved => closed