tracker issue : CF-4204874

And here's a bonus bug: if I refresh my test file twice in a row quickly, I get this error from the application.lock() line: ava.util.EmptyStackException at java.util.Stack.peek(Stack.java:102) at coldfusion.runtime.NeoPageContext.popFunctionLocalScope(NeoPageContext.java:2077) at coldfusion.runtime.Closure.invoke(Closure.java:133) at coldfusion.runtime.UDFMethod$ArgumentCollectionFilter.invoke(UDFMethod.java:447) at coldfusion.filter.FunctionAccessFilter.invoke(FunctionAccessFilter.java:95) at coldfusion.runtime.UDFMethod.runFilterChain(UDFMethod.java:398) at coldfusion.runtime.UDFMethod.runFilterChain(UDFMethod.java:371) at coldfusion.runtime.UDFMethod.invoke(UDFMethod.java:287) at coldfusion.runtime.CfJspPage._invokeUDF(CfJspPage.java:3928) at coldfusion.runtime.CfJspPage._invokeUDF(CfJspPage.java:3908) at coldfusion.runtime.CfJspPage._invoke(CfJspPage.java:3400) at coldfusion.runtime.CfJspPage._invoke(CfJspPage.java:3357) at cfindex2ecfm434543785.runPage(C:\sandbox\closurememtest\index.cfm:9)

Comment by Bradley W.

The error in my previous comment has been logged with a separate test case here: https://tracker.adobe.com/#/view/CF-4206045 Also see this related ticket: https://tracker.adobe.com/#/view/CF-4206046

Comment by Bradley W.

Thanks, Bradley. The moral is loud and clear: use Var scope wherever possible!

Vote by A. B.

Hi Bradley, Your findings notwithstanding, the test-code you've used seems to me to be ambiguous as CFML syntax. In fact, I wonder whether ColdFusion might be behaving as expected, that is, correctly. The closure is transient by definition. It should no longer exist after it has returned. Yet you persist it in the application scope. Even if we take that for granted, your code basically instantiates, then runs, 1000000 distinct functions, each persisted in application scope. Then the contract is that 1000000 functions must run here. The way I see it, should an outside thread come in here, it would have to hurdle over 1000000 named locks. Putting that aside, might the use of application scope be the problem? Why would you persist a named lock as an application-scoped variable anyway? A named lock is implicitly application-scoped to start with. In any case, if there was indeed a memory problem with closure, then the following code, equivalent to yours, would also reproduce the issue. <cfscript> // Only create the closure on the first call if( !structkeyExists( variables, 'lock' ) ) { variables.namedLock = function(){ lock name="myLockName" type="exclusive" timeout="10" {} }; // Call the closure one million times. for (i = 1; i <= 1000000; i++) { variables.namedLock(); } } </cfscript>

Comment by A. B.

Please read "if( !structkeyExists( variables, 'namedLock' ) ) { "

Comment by A. B.

Hi A.B., you are confused on a number of points. > The moral is loud and clear: use Var scope wherever possible! This has nothing at all to do with var scoping. It is about a memory leak in the underlying Java classes that implement closures in CFML. > the test-code you've used seems to me to be ambiguous as CFML syntax. How so? This is just normal CF Code. Perhaps you're not used to seeing functional programming, but there's nothing invalid about it. > In fact, I wonder whether ColdFusion might be behaving as expected, that is, correctly. No, CF is behaving incorrectly. I'm not sure on what grounds you would say otherwise. > The closure is transient by definition Incorrect. A closure is simply a variable like a string, array, or struct. It's persistence is determined entirely by the scope I place it in. > It should no longer exist after it has returned Again, incorrect. Executing a closure doesn't make it disappear any more than using a string makes it go away! Now you may be thinking of the function local scope inside of the invocation, but that is not being looked at here in this example. > Yet you persist it in the application scope. And? I placed the closure in the application scope so it would persist across requests (note my instructions are to run the page 5 times). There is nothing wrong with this. Remember, a closure (and UDFs in general) are simply a variable like a string, array, or struct which can also be placed in any scope. A closure also just so happens to be invocable. > your code basically instantiates, then runs, 1000000 distinct functions, each persisted in application scope. Again, incorrect. There is only ONE closure instance, invoked 1 million times. The code that creates the closure is not inside the loop. While the invocations each have their own function local scope that isn't relevant there. Behind the scenes there is an instance of a Java class called "Closure" that represents our UDF and that is where the memory leak occurs. > The way I see it, should an outside thread come in here, it would have to hurdle over 1000000 named locks. This statement makes no sense. All of the closure invocations run in a single thread. Also, they run serially so the name lock really doesn't make a difference. This is just an example to show the memory leak. If you want to see a real life use of this exact scenario, follow the link I provided in the ticket description. > might the use of application scope be the problem? No, it isn't. I only put it there so the same closure instance would be used across all 5 page refreshes. The exact same behavior would happen if you placed the closure in the variables scope and increased the loop to 5 million. > Why would you persist a named lock as an application-scoped variable anyway? You are confused about what a named lock is. It is not a variable instance, it is a flow/locking control construct. The CFML named lock never directly touchese the variables scope. The tag instance is something very deep inside of the CF engine and behind the scenes that is created for each closure invocation and stored inside the java object used to implement the closure. > if there was indeed a memory problem with closure, then the following code, equivalent to yours, would also reproduce the issue. And it does, like I said above. I think you have gotten way too hung up on the fact that I used the application scope. ANY closure running a tag and persisted in ANY scope and called over and over will leak memory. That is all.

Comment by Bradley W.

There is something common to your remarks. You have in most cases asked your own question. You impute them to me, then proceed to answer, taking ad hominem shots at me along the way. >> This has nothing at all to do with var scoping. It is about a memory leak in the underlying Java classes that implement closures in CFML. I am astonished you should say that Var has nothing at all to do with this. Var scoping is relevant whenever you define a variable within a function in ColdFusion. It helps in marking objects for garbage collection, hence in reducing the risk of memory leaks. >> How so? This is just normal CF Code. Perhaps you're not used to seeing functional programming, but there's nothing invalid about it. Your comment about Functional Programming is, in my opinion, a bit presumptuous. In any case, it is besides the point, as Functional Programming has nothing to do with my remark. I didn't say your test code is invalid; I said it is ambiguous. There can be ambiguity in any programming paradigm or programming language. >> Incorrect. A closure is simply a variable like a string, array, or struct. It's persistence is determined entirely by the scope I place it in. You again presume here. I don't wish to split hairs, but I have to say that your argumentation strays away from the point I made. I know what "a closure" is. My remark was not about closures in general. It was specifically about "the closure" in your test code. >> Again, incorrect. Executing a closure doesn't make it disappear any more than using a string makes it go away! Now you may be thinking of the function local scope inside of the invocation, but that is not being looked at here in this example. Once again your argument is beside the point. No, I am not thinking of function-local. Neither did I say the closure goes away. I said the function body is transient. As you mention string, let me explain, using string. Consider <cfscript> someFunction=function(){ someString="some string"; }; someFunction(); </cfscript> By transience, I do not mean that executing someFunction() makes the variable someFunction or someString go away. What I do mean is that executing someFunction() makes the function body - between curly brackets - transient. >> I placed the closure in the application scope so it would persist across requests (note my instructions are to run the page 5 times). There is nothing wrong with this. Remember, a closure (and UDFs in general) are simply a variable like a string, array, or struct which can also be placed in any scope. A closure also just so happens to be invocable. I agree. >> And? As I have just said, the function body (the code within the curly brackets) is transient when the function is run. I therefore consider it mind-boggling that you would include in it the code for a named lock. Given that a named lock may have a scope that transcends the application's scope. >> Again, incorrect. There is only ONE closure instance, invoked 1 million times. The code that creates the closure is not inside the loop. While the invocations each have their own function local scope that isn't relevant there. Behind the scenes there is an instance of a Java class called "Closure" that represents our UDF and that is where the memory leak occurs. Here we go. You ask your own question, impute it to me, then proceed to answer it. I didn't say there are 1000000 closure instances. I said instead, "your code basically instantiates, then runs, 1000000 distinct functions". >> This statement makes no sense. All of the closure invocations run in a single thread. Also, they run serially so the name lock really doesn't make a difference. This is just an example to show the memory leak. Let's wind it back. I had said, "should an outside thread come in here, it would have to hurdle over 1000000 named locks". If this makes no sense to you, then it may in fact be a reflection on your code. Your code defines named locks. The main use of named locks in ColdFusion is to synchronize access by multiple threads. Hence my presumption that you envisage a scenario where multiple threads will have access to the named lock. >> You are confused about what a named lock is. It is not a variable instance, it is a flow/locking control construct. The CFML named lock never directly touchese the variables scope. The tag instance is something very deep inside of the CF engine and behind the scenes that is created for each closure invocation and stored inside the java object used to implement the closure There we go again. Another ad hominem shot. Take another look. Nowhere did I say or suggest that a lock is a variable instance. I was referring to this code: application.lock = function(){ lock name="myLockName" type="exclusive" timeout="10" {} }; The basic functionality of the closure is a named lock. You have stored it in application scope. Which got me wondering, "Why would you persist a named lock as an application-scoped variable anyway?". >> I think you have gotten way too hung up on the fact that I used the application scope. Not really. It is more the way you implerment the named lock. The code one uses to reproduce an issue in a programming language should itself be beyond reproach. In my opinion your test code raises a number of questions of its own. >> ANY closure running a tag and persisted in ANY scope and called over and over will leak memory. That is all. I praise you for bringing this to light. It is an important issue, and needs to be solved urgently.

Comment by A. B.

A. B. I'm not going to keep replying here as it seems you just want to argue. I will say it is quite improper to accuse me of making ad hominem attacks against you as I have done no such thing. Pointing out an incorrect or false statement is challenging a position some holds on the subject, not attacking them as a person. That is, by definition. literally the opposite of an ad hominem attack.

Comment by Bradley W.

Hi Bradley, It is easy to claim that a statement is false or an opinion incorrect. It's another matter to show why. Presumptuous remarks such as "you are confused about ...", "you're not used to seeing ...", "you may be thinking of ...", and so on, are no substitute for technical argument. I don't "just want to argue". I took the time to respond to the points you made in the hope our discussion will shed more light on the bug, expediting its fix.

Comment by A. B.

I don't understand what is even in contention with anything on this ticket. If you create a closure, it becomes a static variable. If it is scoped in to the application, it becomes an application-wide static variable. As such, it should be thread-safe and available to run as many times as it needs to and should re-create a new memory footprint every time it is invoked.

Comment by Jonathan C.

Typo above: should be *should not re-create*

Comment by Jonathan C.

+1 to Brad and Jonathan's remarks. A memory leak in a bedrock language function should not require 6+ months to address.

Comment by Samuel K.