https://quality.embarcadero.com/browse/RSP-12462
https://quality.embarcadero.com/browse/RSP-12462
TThreadPool worker thread holds reference to last executed task
Closed with the following comment: "So? Why is this an issue?"
Resolution: Works As Expected
Does anyone working at Embarcadero still know how to program?
https://quality.embarcadero.com/browse/RSP-12462
TThreadPool worker thread holds reference to last executed task
Closed with the following comment: "So? Why is this an issue?"
Resolution: Works As Expected
Does anyone working at Embarcadero still know how to program?
https://quality.embarcadero.com/browse/RSP-12462
Think I'll have to wait with disputing this one. Can't get myself to write anything nice...
ReplyDeleteHave you tested this behaviour in .NET's Task Parallel Library? I think the thread pool should dispose the task as soon as possible once the task cannot run again.
ReplyDeleteHorácio Filho No, in any case I don't think .Net's behavior in this case is relevant given the differences in memory management.
ReplyDelete>Once the work has completed, the task container is irrelevant.
ReplyDeleteIf the task container is irrelevant once the work has completed, why hold on to it :-P
Nicholas Ring I get your joke, but that part is actually a bit subtle. The reason it's held on to is because the task reference is assigned to a local variable which does not go out of scope until the threadpool is destroyed. So the fix is to explicitly nil the local variable after execution.
ReplyDeleteAsbjørn Heid I disputed it for you, writing nicely :) The other option is to flag Marco Cantù who I'm sure will have a look at it.
ReplyDeleteDavid Millington Thanks, I'll add some additional comments in a few days when I've calmed down a bit...
ReplyDeleteActually my first comment on the issue says it all, but I'll add another example just in case there's someone with a clue still left at Embarcadero...
ReplyDeleteI feel your pain. :(
ReplyDeleteAsbjørn Heid It was half joke, half serious... I was just using their reasoning about the completed task container being irrelevant to let go of the task container - why keep hold on an irrelevant piece of data/object/etc ?
ReplyDeleteYou will also like to know that another TTask report has been close satisfactory - https://plus.google.com/u/0/102906537220705979718/posts/BcATYULvz1F
Nicholas Ring Gotta love the effort they put into writing the reasons behind their decision...
ReplyDeleteThis is what I wrote in QP:
ReplyDelete====
> the resolution shows a big misunderstanding of the problem
That's a very bold statement. The resolution is based on the fact the original code (and even the new one) indicate some unexpected sequence of events that is not really so unexpected, given the reference counting model underlying interfaces and anonymous methods.
If the issue is the anonymous method capturing a variable and not releasing it, that would be a reasonable demo to create and submit. But it that the case? It might as well be, and that would be the issue.
Submitting a "potential" issue is not an actual way to get attention, how bad the code might seem.
====
To me the bug report as is has been handled and closed correctly, with QA opening it and a (senior) engineer indicating he sees no harm in the code. The specific destruction sequence shown in the demo is as expected. OK to submit a scenario with an actual bug in this or a separate entry.
Also "tasks are likely anonymous methods which capture references": Does the task release the anonymous method upon completion? If no, the situation is not so alarming. If this happens, than that might be a better solution, depending on the scenario.
The specific destruction sequence shown in the demo is as expected.
ReplyDeleteTo me it was completely unexpected. That none of you think that holding on to an anonymous method until after the final "end." is an issue is utterly shocking.
To me this clearly means that I cannot trust any new library that Embarcadero publishes.
Asbjørn Heid Who said that? Oh, well, I did. Sorry. Actually meant the opposite, was supposed to be: "Does the task release the anonymous method upon completion? If so, the situation is not so alarming."
ReplyDeleteI agree that leaking the anonymous method is a big deal, but this is not the issue in the bug report or the demo code.
Marco Cantù So if I make a custom task I should not expect it to get freed until after "end.", ok, nice to know.
ReplyDeleteWhere is this completely unexpected behavior documented?
Because that was my angle with this report. The whole thing started with "leaking" anonymous methods, but when I saw why, I thought the correct approach was to not "leak" the ITask.
Why?
Because it is entirely possible to create your own TTask descendant, and then it will not be destroyed when one would typically expect.
And by not leaking the ITask, it would naturally fix the issue of not leaking the anonymous methods.
Now, even if Embarcadero thinks it's entirely fine to hold on to ITask instances (which is quite indiciative of the new quality standards), holding on to anonymous methods should be met with something more than a "so?".
Oh well, I think I'll give up on Quality Portal. I have more fun things to do than to try to convince clueless devs.
I'm not saying that the issues are not relevant, just that I feel the anonymous method leak (if it is there) as a more severe issue. I'll reopen the report, to have a second look, but I think a better test case could have helped (still, we took way to much time returning it, so we didn't manage it properly). About the "clueless devs" I'd have objections...
ReplyDeleteMarco Cantù The anonymous leak was mentioned in the first comment, before the report was reported internally.
ReplyDeleteAny dev with a clue would, if they didn't immediately think about the consequences from the title alone, read that comment and see the impact.
Any dev with a clue would see "oh shit this is bad".
Any dev with a clue would, if the angle of the report is determined to be wrong (leaking ITask is fine, anonymous method is not), not reply with a "so?"!!!
They would instead refocus the report, or, if that is inconvenient due to the repliction, close the report and ask for it to be re-opened focusing on the anonymous method issue (ideally open a new one internally and sync up with the publci QP report afterwards).
That's why I can immediately tell the dev is clueless.
Let me clarify why I think the "so?" reply is so infuriating (I guess you noticed this has fired me up):
ReplyDelete1. If holding on to the ITask is deemed acceptable by the library, then this is a serious documentation issue as it will likely bite those who write their own implementation of ITask. If so the report should have been reclassified.
2. There is no other comment at all regarding the anonymous method "leak" side effect. Holding on to an anonymous method is absolutely not something one says "so?" to.
Thanks Marco Cantù. This really pissed me off for some reason.
ReplyDelete