View Issue Details
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0035210||Lazarus||TAChart||public||2019-03-09 01:21||2019-03-31 16:15|
|Reporter||Marcin Wiazowski||Assigned To||wp|
|Product Version||2.1 (SVN)||Product Build||60628|
|Target Version||2.2||Fixed in Version|
|Summary||0035210: TAChart: two problems with notification broadcasting|
|Description||When 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.
|Tags||No tags attached.|
|Fixed in Revision||60657, 60801|
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)
> 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.
Reproduce2.zip (2,469 bytes)
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:
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
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)
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:
In the broadcaster, there is a loop, which effectively works as:
i := 0;
while i < NotificationList.Count do
Line A: send notification to NotificationList[i];
Line B: Inc(i);
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:
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...
||Applied, thanks. The observation that the reported behavior can block the IDE made the decision easier.|
||Fix confirmed. Thanks!|
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.
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)
|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|