View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0003301 | JEDI VCL | 00 JVCL Components | public | 2005-11-04 06:29 | 2005-11-27 03:43 |
| Reporter | Kiriakos | Assigned To | AHUser | ||
| Priority | normal | Severity | major | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Product Version | 3.00 | ||||
| Target Version | Fixed in Version | 3.10 | |||
| Summary | 0003301: Memory loss and crashes in JvChangeNotify | ||||
| Description | I discovered the following problem with FastMM4: When the property FreeOnTerminate is True the JvChangeThread is not freed and crashes can occur. Analysis: Assume that in your program you so somewhere: JvChangeNotify.Active := False; // do something JvChangeNotify.Active := True; Setting Active to False will trigger the following code in TJvChangeNotify.SetActive: if FThread <> nil then begin FThread.Terminate; if FreeOnTerminate then FThread := nil else begin FThread.WaitFor; FreeAndNil(FThread); end; end; Whereas Seting Active to True will create a JvChangeThread and assign it to FThread; At the same time when the original JvChangeThread gets destroyed DoThreadTerminate is called which sets FThread to nil; This may well happen after the new JvChangeThread has been created. So FThread will be nil when the JvThread is actually alive and active. Next time you set the Active property to False the JvActiveThread will not be destoyed with the result of having multiple JvChangeThreads active. Solution: Remove DoThreadTerminate and all references to it. Further problems: When the JvChangeNotify gets destroyed is sets its Active property to false which will eventually destroy the JvChangeThread. However the destruction of the thread will happen after JvChangeNotify will have been destroyed. The Execute method of the thread will call its FNotify method which in turn calls the FNotify of the JvChangeNotify which will be an invalid pointer. Suggestion: In SetActive add the following line before calling FThread.Terminate; (2 places): FThread.OnChangeNotify := nil; For futher safety, in procedure TJvChangeThread.Execute modify the 5th line to if (I >= 0) and (I < FCount) and not Terminated then Even with the above suggestions there is still a chance when the Interval is long enough that TJvChangeThread.Execute will get stuck in WaitForMultipleObjects for too long and the main thread will have finished before this thread is finished. | ||||
| Tags | No tags attached. | ||||
|
|
Further to my earlier note, I found it benefitial to add a statement Sleep(200) or even Sleep(0) in the FormDestroy of the main application, which would give the time to the JvChangeThread (and other such threads) to terminate, so that memory loss would be avoided. |
|
|
Fixed in CVS. SetActive() converts every FreeOnTerminte thread into a non-FreeOnTerminate thread and destroys it immediately after a WaitFor call. |
|
|
Andreas, Your solultion eliminates the difference betweeen setting the FreeOnTerminate to True or False. There was a reason for having the FreeOnTerminate flag: to be able to call SetActive from the JvChangeNotify event handler (see Peter's note on this). Your change will break existing code resulting in the program deadlocking. Just think about it. a) The thread execute method synchronizes into the main thread to execute the JvChangeNotify event hanldler. b) The event handler calls SetActive(False) c) SetActive(False) waits for the thread to terminate. Hence deadlock! Solution: A combination of my suggestions in earlier note and your solution would solve all problem. a) Reverse your changes! b) Implement the changes I suggested. (including removing DoOnTerminate which I think serves no purpose) c) On component destruction do what you are currently doing in SetActive (set FreeOnTerminate to False, Call SetActive(False) and then WaitFor the thread to terminate if the FreeOnTerminate was True. |
|
|
Andreas, Your solultion eliminates the difference betweeen setting the FreeOnTerminate to True or False. There was a reason for having the FreeOnTerminate flag: to be able to call SetActive from the JvChangeNotify event handler (see Peter's note on this). Your change will break existing code resulting in the program deadlocking. Just think about it. a) The thread execute method synchronizes into the main thread to execute the JvChangeNotify event hanldler. b) The event handler calls SetActive(False) c) SetActive(False) waits for the thread to terminate. Hence deadlock! Solution: A combination of my suggestions in earlier note and your solution would solve all problem. a) Reverse your changes! b) Implement the changes I suggested. (including removing DoOnTerminate which I think serves no purpose) c) On component destruction do what you are currently doing in SetActive (set FreeOnTerminate to False, Call SetActive(False) and then WaitFor the thread to terminate if the FreeOnTerminate was True. |
|
|
Previous bugfix reverted and reimplemented. Fixed in CVS. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2005-11-04 06:29 | Kiriakos | New Issue | |
| 2005-11-11 16:49 | Kiriakos | Note Added: 0008085 | |
| 2005-11-11 16:49 | Kiriakos | Note Edited: 0008085 | |
| 2005-11-22 04:28 | AHUser | Status | new => resolved |
| 2005-11-22 04:28 | AHUser | Resolution | open => fixed |
| 2005-11-22 04:28 | AHUser | Assigned To | => AHUser |
| 2005-11-22 04:28 | AHUser | Note Added: 0008111 | |
| 2005-11-27 02:49 | Kiriakos | Status | resolved => feedback |
| 2005-11-27 02:49 | Kiriakos | Resolution | fixed => reopened |
| 2005-11-27 02:49 | Kiriakos | Note Added: 0008169 | |
| 2005-11-27 02:52 | Kiriakos | Note Added: 0008170 | |
| 2005-11-27 03:43 | AHUser | Status | feedback => resolved |
| 2005-11-27 03:43 | AHUser | Resolution | reopened => fixed |
| 2005-11-27 03:43 | AHUser | Note Added: 0008171 |
