<< Back to previous view

[ASYNC-138] Go blocks leak memory Created: 10/Aug/15  Updated: 22/Feb/17  Resolved: 22/Feb/17

Status: Closed
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Critical
Reporter: Brian Lubeski Assignee: Unassigned
Resolution: Completed Votes: 14
Labels: go, memory
Environment:

clojure 1.7.0
core.async 0.1.346.0-17112a-alpha
Java HotSpot(TM) Client VM 1.8.0_31-b13


Attachments: Text File 0001-ASYNC-138-allow-for-clearing-of-closed-over-locals.patch     Text File 0001-ASYNC-138-allow-for-clearing-of-closed-over-locals-v2-nested.patch     Text File 0001-ASYNC-138-allow-for-clearing-of-closed-over-locals-v2.patch     Text File 0001-ASYNC-138-allow-for-clearing-of-closed-over-locals-v3.patch     Text File 0001-ASYNC-138-allow-for-clearing-of-closed-over-locals-v4.patch     Text File 0001-ASYNC-138-allow-for-clearing-of-closed-over-locals-v5.patch     Text File 0001-ASYNC-138-allow-for-clearing-of-closed-over-locals-v6.patch    
Patch: Code and Test
Approval: Vetted

 Description   

The following example, after running for a few minutes, generates an OutOfMemoryError.

(let [c (chan)]
  (go (while (<! c)))
  (let [vs (range)]
    (go
      (doseq [v vs]
        (>! c v)))))

By contrast, the following example will run indefinitely without causing an OutOfMemoryError.

(let [c (chan)]
  (go (while (<! c)))
  (go
    (let [vs (range)] 
      (doseq [v vs]
        (>! c v)))))

The only significant difference I see between the two examples is that the (range) is created outside the go block in the first example but is created inside the go block in the second example. It appears that the go block in the first example is referencing vs in such a way as to prevent if from being garbage-collected.

This behavior is also be the cause of ASYNC-32.

Approach:
The attached patch applies a transformation from

(let [a large-seq] (loop [..] .. a ..))
where the loop exemplifies the state machine loop created by `go`, into
(let [a large-seq a' (^:once fn* [] a)] (loop [..] .. (let [a (a')] ..) ..))

which allows the closed over locals to cross into the state machine loop without getting held. While this might look unsafe, because of how the `go` loop state machine works, we're guaranteed that `(a')` will only be invoked on the first iteration of the loop.
The patch looks way more complex than it actually is, because it needs to deal with: renaming locals so that shadowing of special symbols works properly, preserving locals type hints and handle nested go blocks, where the value of `&env` will be provided by tools.analyzer rather than from Compiler.java

There are two different type propagations that we need to do, one in case of non primitive objects and one in the case of primitive values:

  • if we're propagating type info for a non primitive object, the transformation is as follow:
    (let [a ^Klass my-obj] (loop [..] .. a ..))
    ->
    (let [a ^Klass my-obj a' (^:once fn* [] a)] (loop [..] .. (let [^Klass a (a')] ..) ..))
  • if we're propagating type info for a primitive local, a type hint won't do – we actually need to unbox the value out of the thunk, so the transformation will be:
(let [a (int x)] (loop [..] .. a ..))
->
(let [a (int x) a' (^:once fn* [] a)] (loop [..] .. (let [a (clojure.core/int (a'))] ..) ..))

Patch: 0001-ASYNC-138-allow-for-clearing-of-closed-over-locals-v6.patch



 Comments   
Comment by Nicola Mometto [ 15/Dec/15 9:34 AM ]

attached is a WIP patch to fix this issue, would appreciate some testing on this

Comment by Nicola Mometto [ 15/Dec/15 5:52 PM ]

CLJ-1744 contributes to this bug

Comment by Nicola Mometto [ 15/Dec/15 6:23 PM ]

Updated patch to preserve type info

Comment by Nicola Mometto [ 17/Dec/15 10:16 AM ]

Note that the current patch breaks nested `go` blocks at compile time since the nested `go` block will be macroexpanded by tools.analyzer.jvm rather than Compiler.java and this &env will be different.

Is this something we want to support? There seem to be a lot of other cases that break when nesting `go` blocks.

If this is something that we do want to support 0001-ASYNC-138-allow-for-clearing-of-closed-over-locals-v2-nested.patch fixes this

Comment by Jan Rychter [ 21/Dec/15 6:18 AM ]

This just bit me badly when doing data processing work — I never expected that (async/to-chan (line-seq rdr)) can be the culprit of my heap getting exhausted in 30 seconds. Took quite some time to narrow it down. Thanks to Nicola for letting me know about this bug on Slack.

Comment by Alex Miller [ 13/Feb/17 3:31 PM ]

Nicola, not sure how to read your comments above re the patches. Most of the other cases of nested go blocks failing that I see in the tracker are related to the exception handling bugs in ASYNC-169. Do you have any hesitancy over your patch for the nested stuff?

Comment by Nicola Mometto [ 13/Feb/17 3:40 PM ]

Alex, I think the only reason why I proposed the -nested patch as an alternative rather than as a main patch is that it adds complexity to the patch to handle an unlikely scenario. I think we should go with the -nested patch, but if understanding the patch for screeners is more important than handling weird edge cases, the other patch is easier

Comment by Alex Miller [ 13/Feb/17 3:55 PM ]

Tests don't pass with -nested patch.

Caused by: java.lang.ClassCastException: clojure.lang.Symbol cannot be cast to java.util.Map$Entry
	at clojure.lang.APersistentMap$ValSeq.first(APersistentMap.java:232)
	at clojure.lang.RT.first(RT.java:653)
	at clojure.core$first__4110.invoke(core.clj:55)
	at clojure.core.async.impl.ioc_macros$state_machine.invoke(ioc_macros.clj:1107)
	at clojure.core.async.ioc_macros_test$runner.doInvoke(ioc_macros_test.clj:24)
Comment by Nicola Mometto [ 13/Feb/17 5:04 PM ]

updated patch making all the tests run

Comment by Nicola Mometto [ 14/Feb/17 4:49 AM ]

updated patch with an additional testcase making sure nested go blocks work properly after the transformation, also bumping tools.analyzer.jvm to the last version as that includes a fix for nested t.a calls

Comment by Alex Miller [ 14/Feb/17 10:40 AM ]

Applied.

Comment by Alex Miller [ 15/Feb/17 3:36 PM ]

I am reopening this and reverting the change for the moment. This example fails when using Clojure 1.9.0-alpha12+ (anything after the commit for CLJ-1224).

(require '[clojure.core.async :refer (go)])
(defprotocol P
  (x [p]))

(defrecord R [z]
  P
  (x [_]
    (go
      (loop []
        (recur)))))

CompilerException java.lang.UnsupportedOperationException: Can't type hint a primitive local

I believe this is due to the code in the patch related to applying meta to the local binding, specifically this comes into interaction with the code now generated in hasheq after CLJ-1224.

Comment by Nicola Mometto [ 15/Feb/17 4:58 PM ]

Updated the patch to handle with primitive type hints.
In case of primitive type hints, rather than emitting

(let [^String a (a')] ..)
we emit
(let [a (clojure.core/int (a'))] ..)
as hinting is not valid and proper unboxing is required

Comment by Alex Miller [ 15/Feb/17 8:12 PM ]

Did you mean to have ^String and clojure.core/int in that example?

Comment by Nicola Mometto [ 16/Feb/17 3:12 AM ]

yes, to showcase the two different transformations we do in case of primitive or non primitive type propagation. I'll expand on that in the ticket description so it's clearer what i meant

Comment by Alex Miller [ 17/Feb/17 9:10 AM ]

Applied v5 version.





[ASYNC-92] go macro removes binding forms that are intialized with logical false value Created: 03/Oct/14  Updated: 13/Feb/17

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Oleh Palianytsia Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: go
Environment:

org.clojure/core.async "0.1.346.0-17112a-alpha"


Attachments: File fix-async-92.diff    
Approval: Triaged

 Description   
(require '[clojure.core.async :as a])

(a/go (let [a nil] (a/alts! (if a <whatever> <whatever>)))) // Unable to resolve a
(a/go (let [a nil] (a/<! (if a <whatever> <whatever>))) // Unable to resolve a

Seems that 'go' macro removes falsely initialized symbols that are used as channels, because
in both cases there's exception, that says " Unable to resolve symbol: a in this context".



 Comments   
Comment by Willy Blandin [ 17/Oct/14 12:19 PM ]

Confirmed.
Bug was introduced between 0.1.278.0-76b25b-alpha and 0.1.295.0-9ea6ef-alpha.

Comment by Willy Blandin [ 17/Oct/14 12:27 PM ]

Worked around with:

(defmacro workaround-async-92
  "Hack to workaround core.async bug
   cf. http://dev.clojure.org/jira/browse/ASYNC-92"
  []
  ;; has to be a list
  `(do nil))

(let [a (workaround-async-92)]
  ...)
Comment by Leon Grapenthin [ 23/Oct/14 11:55 AM ]

modifies two methods of the RawCode inst so that they check:collected-locals in locals via contains? before ignoring them

Comment by Ghadi Shayban [ 23/Oct/14 5:19 PM ]

Hi Leon, thanks for the patch. Can you fill out a Contributor Agreement? http://clojure.org/contributing

Comment by Leon Grapenthin [ 24/Oct/14 7:17 AM ]

I did, yesterday. Got an automatic confirmation email saying Rich Hickey signed it. Anything else I should do with it?





[ASYNC-29] 50 parallel blocking takes in go-blocks as loop-binding or recur argument break go Created: 28/Oct/13  Updated: 29/Oct/13  Resolved: 29/Oct/13

Status: Resolved
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Leon Grapenthin Assignee: Rich Hickey
Resolution: Completed Votes: 0
Labels: bug, go, go-macro
Environment:

JVM



 Description   

This problem raises the question whether what the implications are of using <!! and >!! inside of go and whether it is legal.

What if I provide API functions that are using <!! >!! and may be invoked from within go?
Should I then also provide !! and ! versions?

Please evalute this code at the REPL to reproduce the problem: https://www.refheap.com/20225



 Comments   
Comment by Leon Grapenthin [ 29/Oct/13 9:46 AM ]

I'd like to rephrase my question: Do I have to provide {!} and {!!} macros that do the same thing and use {<!} {>!} and {<!!} {>!!} respectively? Would it make sense for core.async to provide a defasync macro that creates those two versions from the same body where you could for example use {<!!!} and {>!!!}, {alts!!!} and so on so that they would be replaced by a postwalk before the macro is defined defined twice with {!} and {!!} appended to the name? Or are there other plans of abstraction? [Quoting symbols because of jira markup]

Comment by Rich Hickey [ 29/Oct/13 9:55 AM ]

You shouldn't be using <!! and >!! in library code. We may at some point be able to detect at runtime their use in go blocks and throw errors, but currently do not.





[ASYNC-24] dosync does not work inside go macro Created: 15/Sep/13  Updated: 31/Oct/13  Resolved: 31/Oct/13

Status: Closed
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Michael Ummels Assignee: Timothy Baldridge
Resolution: Completed Votes: 0
Labels: bug,, go
Environment:

Clojure 1.5.1 in the JVM, core.async-0.1.0-20130827.050117-78"



 Description   

The following code results in an exception:

=> (go (dosync nil))

CompilerException java.lang.RuntimeException: No such var: clojure.core/runInTransaction



 Comments   
Comment by Michael Ummels [ 09/Oct/13 2:05 AM ]

Fixed in master. Please close.

Comment by Timothy Baldridge [ 31/Oct/13 9:20 AM ]

Fixed in master





[ASYNC-20] Case macro in go block ignores default expression Created: 20/Aug/13  Updated: 27/Sep/13  Resolved: 27/Sep/13

Status: Closed
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Leon Grapenthin Assignee: Timothy Baldridge
Resolution: Completed Votes: 0
Labels: bug, go
Environment:

Clojure 1.5.1 in the JVM, core.async-0.1.0-20130819.220150-69



 Description   

To reproduce the error use this code:
(go (case true
false false
nil))

An IllegalArgumentException will be thrown (as if there was no default expression).
Exception in thread "async-dispatch-46" java.lang.IllegalArgumentException: No matching clause: true



 Comments   
Comment by Timothy Baldridge [ 27/Sep/13 8:34 AM ]

fixed in: https://github.com/clojure/core.async/commit/f9b24552cf4f6b8b86f3377c7b7a42a618a9fd76





Generated at Sat Feb 25 12:10:19 CST 2017 using JIRA 4.4#649-r158309.