tracker issue : CF-4206045

select a category, or use search below
(searches all categories and all time range)
Title:

Closure instances are not thread safe to execute in separate threads

| View in Tracker

Status/Resolution/Reason: To Fix//BugVerified

Reporter/Name(from Bugbase): Bradley W. / ()

Created: 11/21/2019

Components: Language

Versions: 2016,2018

Failure Type: Data Corruption

Found In Build/Fixed In Build: Update 5 /

Priority/Frequency: Normal / Unknown

Locale/System: English / Platforms All

Vote Count: 12

Problem Description:

If a closure is declared once and then shared by two threads that run it at the same time, it is not thread safe.  The Closure keeps a reference to the page context where it was declared and the NeoPageContext.popFunctionLocalScope() method is not thread safe.

Access to the function local scopes in the page context need to be synchronized for a given page context object.  This error also happens on CF 2018 update 12 and CF 11 update 19

Steps to Reproduce:

Tail your ColdFusion-out log while running this code

application.udf = function() {}
cfloop( from="1", to="10000", index="i" ) {  
	thread name='test#i#' {
		try {
			application.udf();
		} catch( any e ) {
			writeDump( var=e.stacktrace, output="console" );
		}
	}
}

Actual Result:

Several of the threads will throw this error, which will show in the console output.

 java.util.EmptyStackException
        at java.base/java.util.Stack.peek(Unknown Source)
        at coldfusion.runtime.NeoPageContext.popFunctionLocalScope(NeoPageContext.java:2089)
        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:4132)
        at coldfusion.runtime.CfJspPage._invokeUDF(CfJspPage.java:4112)
        at coldfusion.runtime.CfJspPage._invoke(CfJspPage.java:3604)
        at coldfusion.runtime.CfJspPage._invoke(CfJspPage.java:3561)
        at cfindex2ecfm1403424347$func_CFFUNCCFTHREAD_CFINDEX2ECFM14034243471.runFunction(C:\sandbox\adobeThreading\index.cfm:11)

Expected Result:

10,000 threads should run the closure with no errors.

Any Workarounds:

Single thread the execution of the UDF, which of course, makes the threading moot.

Attachments:

Comments:

Related ticket for UDF instances: https://tracker.adobe.com/#/view/CF-4206046 This ticket is a blocker for the cbstreams library which shares Closures across threads but this bug could bite any CF codebase sharing a UDF.
Comment by Bradley W.
31828 | November 21, 2019 03:07:04 PM GMT
Thread-safety is a major spoil-sport. So I am voting for the Adobe team to look into this.
Vote by A. B.
31831 | November 22, 2019 01:34:45 PM GMT
My comments in your other ticket (https://tracker.adobe.com/#/view/CF-4204874) are relevant here too. The following code has some ambiguity. the closure here is transient. Should you then be storing it in a persistent scope? I am talking about the code, application.udf = function() {} The left-hand side persists after evaluation, as it is an application-scoped variable. Whereas the right-hand side is garbage-collected after it runs.
Comment by A. B.
31829 | November 22, 2019 01:44:46 PM GMT
A.B. I have replied categorically on the other ticket.
Comment by Bradley W.
31830 | November 22, 2019 06:47:06 PM GMT
Hi Bradley, On re-reading this, I would admit that my own remark contains some ambiguity. I seem to implicitly suggest that the code application.udf = function() {} is erroneous. If so, my bad. The code is, of course, correct. What stumps me is the design. Let me explain. Consider application.appVar1 = session.myVar1; application.appVar2 = variables.myVar2; Both are perfectly correct code - assuming session.myVar1 and variables.myVar1 exist. However, the design of the left-hand side is for persistence throughout the application and for the duration of the application. (It implies, "You, me or anyone else using this application, shares the same appVar1 and appvar2, globally, possibly till the application ends"). Whereas the design of right-hand side is for locality and temporariness. What I call transience, for want of a better word. (It implies, "I have my myVar1 and myvar2, you have yours and she has hers, on this occasion"). So the question arises why you would want to use such a design. Especially as it could be a distraction in a bug report. A closure represents a function. As such it encapsulates behaviour. Behaviour has, by design, local context. At least in object-oriented programming, the paradigm ColdFusion supports. It is this locality which gives closures their usefulness and power. It therefore wouldn't surprise me if the ColdFusion Team designed closures to have a local context. If they did then that might have something to do with this bug.
Comment by A. B.
31867 | November 24, 2019 08:59:20 AM GMT
Please read "variables.myVar2" in place of "variables.myVar1 "
Comment by A. B.
31868 | November 24, 2019 10:57:44 AM GMT
"So the question arises why you would want to use such a design. " I don't believe your analysis of the code sample is looking at real-world programming examples. CFML Developers have been scoping static methods (UDFs) in to the application scope, for re-use in all templates, since even before script-based components were available. It's a common method of defining UDFs. A closure, when scoped should be treated as a static. The right side, is certainly not "temporal" - unless it is var-scoped for in-method usage, at which point it can be garbage collected when the function is complete. When scoped in to the application, a closure must be static and thread-safe - just like any first class CFML language method. "A closure represents a function. As such it encapsulates behaviour. Behaviour has, by design, local context. " I'm not following your point here, unless you're applying it to the internals of the function/closure, which do, indeed encapsulate behavior. If you apply that logic exclusively to variables which are closures, then you are drastically limiting the way in which they can be used within the language. Since CFML maintains an application scope, unlike languages like PHP, which assemble the entire global scope with each request, it goes without saying that anything which is scoped in to the application should be thread safe. In the case of the closure, that means the function itself would be static and no attempt at garbage collection should be made on the externals ( the right-hand side ) until the variable is destroyed or re-declared. It also, per ticket 4204874, should be able to be invoked a many times as needed - in thread, out of thread, in any piece of code within the application, as many times as necessary, without increasing the memory footprint used to maintain the reference to that closure.
Comment by Jonathan C.
31877 | November 25, 2019 01:00:14 PM GMT
Hi Jonathan, Yes, I agree that "a closure, when scoped should be treated as a static variable". And, yes, given the code application.myClosure = function () {} I agree with your point that application.myClosure "itself would be static and no attempt at garbage collection should be made on the externals ( the right-hand side ) until the variable is destroyed or re-declared" But I would beg to differ on your point about invocation. That is, application.myClosure() You say, "It also, per ticket 4204874, should be able to be invoked a many times as needed - in thread, out of thread, in any piece of code within the application, as many times as necessary, without increasing the memory footprint used to maintain the reference to that closure." I hope for that too, and did vote for the ticket. But I do believe that there is another consideration when closures are invoked. For, closures are environment-aware. I shall explain with an analogy. Please bear with me. Draw a square. Mark two points within it, some distance apart. Join the points by a line. Still within the square, draw a circle around one of the points. Consider the circle to represent a closure, and the point within it a local property. The square represents the immediate external environment containing the closure, say the page-context of a CFM or CFC. The point outside the circle represents a property in the external environment. It is coupled to the local property within the closure. Now, wherever you invoke the closure, you introduce the external environment in which the closure was created. You also revive, at the point where the closure is invoked, the coupling that existed between its local and external environment when it was created. If the property in the external environment had a high memory footprint, So, yes, when you place a closure in application scope, you enable reuse throughout the application. But, in so doing, you also create coupling between separate parts of the application. That is the gist of my remarks about behaviour and design.
Comment by A. B.
31878 | November 26, 2019 06:06:15 AM GMT
Sorry about the half sentence. I am typing in a hurry, as I have to leave for work. Please read "If the property in the external environment had a high memory footprint, that will also be revived at the point of invocation".
Comment by A. B.
31879 | November 26, 2019 06:13:03 AM GMT