Open upvalues can be freed when transferring to another fiber

[munificent]

Take a look at:

class Test {
  static run() {
    var outer = "outer"

    Fiber.new {
      System.gc()
      System.print(outer)
    }.transfer()
  }
}

Test.run()

Here's what happens when you run it:

• A root fiber A is created and executes run().
• Local variable outer is allocated on A's stack.
• A new fiber B is created. The closure for that fiber has an open upvalue pointing to outer on A's stack.
• We transfer to B. At this point, there are no references to A.
• System.gc() frees fiber A.
• The VM tries to load upvalue outer, which now points into freed memory.

:(

The open upvalue stuff works fine for called fibers because the callee retains a reference to the caller and keeps that fiber alive. Transfers aren't so lucky.

"test/api/call_wren_call_root" fails because of this if you have GC stressing on. I'm not sure of the best way to fix this. Wren's upvalues are based on Lua, but Lua does not have this problem because Lua doesn't have transfer(), so I don't think we can learn much from there. A couple of ideas to investigate:

• Make open upvalues store a reference to the fiber whose stack owns the upvalue. This has a memory implication and may make GC and creating closures a little slower.
• Get rid of the notion of open upvalues and always eagerly put upvalues on the heap. This will obviously cause a perf hit.
• Close all of a fiber's open upvalues when the VM transfers away from it. This is appealing because transferring is a rare operation and this localizes the cost there.

I think the latter will work, but I'm filing this issue so I don't forget before I have time to look into this more.

[mhermier]

Maybe resume should be removed then, since it implies scheduling, and implementation is very messy.

Le ven. 8 févr. 2019 à 18:45, Bob Nystrom notifications@github.com a écrit :

Take a look at:

class Test { static run() { var outer = "outer"

Fiber.new {
  System.gc()
  System.print(outer)
}.transfer()

} }

Test.run()

Here's what happens when you run it:

• A root fiber A is created and executes run().
• Local variable outer is allocated on A's stack.
• A new fiber B is created. The closure for that fiber has an open upvalue pointing to outer on A's stack.
• We transfer to B. At this point, there are no references to A.
• System.gc() frees fiber A.
• The VM tries to load upvalue outer, which now points into freed memory.

:(

The open upvalue stuff works fine for called fibers because the callee retains a reference to the caller and keeps that fiber alive. Transfers aren't so lucky.

"test/api/call_wren_call_root" fails because of this if you have GC stressing on. I'm not sure of the best way to fix this. Wren's upvalues are based on Lua, but Lua does not have this problem because Lua doesn't have transfer(), so I don't think we can learn much from there. A couple of ideas to investigate:

• Make open upvalues store a reference to the fiber whose stack owns the upvalue. This has a memory implication and may make GC and creating closures a little slower.
• Get rid of the notion of open upvalues and always eagerly put upvalues on the heap. This will obviously cause a perf hit.
• Close all of a fiber's open upvalues when the VM transfers away from it. This is appealing because transferring is a rare operation and this localizes the cost there.

I think the latter will work, but I'm filing this issue so I don't forget before I have time to look into this more.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/wren-lang/wren/issues/654, or mute the thread https://github.com/notifications/unsubscribe-auth/AY7bldE7gmpCqflP7At5nOAAW0syaZHCks5vLbexgaJpZM4awxt_ .

[munificent]

I do think it's a very powerful, useful feature of Wren that it supports transfer() and true symmetric coroutines. This is what makes it possible to write a scheduler in Wren and in turn what makes it possible for the Wren CLI to use async IO while showing the user a synchronous API. It's honestly one of my favorite features of Wren and the VM.

But this bug is a real bug that should be fixed. :)

[mhermier]

Reading again, it seems I rushed a little at understanding the issue. It is true that an upvalue should preserve the calling context, so the issue is real. I mixed thought, thinking it was involving IO scheduler.