Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

error: cannot marshal fiber with c stackframe #1326

Closed
zevv opened this issue Nov 10, 2023 · 2 comments
Closed

error: cannot marshal fiber with c stackframe #1326

zevv opened this issue Nov 10, 2023 · 2 comments
Assignees

Comments

@zevv
Copy link
Contributor

zevv commented Nov 10, 2023

Reporting for a friend who's too lazy to report this himself.

The snippet below results in

instantiated (ev/call fun)
sleeping...
process exiting with code 0
instantiated (ev/go (short-fn (ev/do-thread (fun))))
error: cannot marshal fiber with c stackframe
  in ev/thread [src/core/ev.c] on line 2852
  in <anonymous> [disruptek.janet] on line 24, column 14

Removing the do makes this problem go away.

(defn spawn-sleep
  []
  (print "sleeping...")
  (let [process (os/spawn ["sleep" "3"] :p {})
        pid (process :pid)]
    (os/proc-close process)
    (printf "process exiting with code %d"
            (process :return-code))))

(defmacro test
  [form]
  ~(let [fib ,form]
     (printf "instantiated %q" ',form)
     (forever
       (match (fiber/status fib)
         :dead
         (break)
         status
         (ev/sleep 1)))))

(do
(def fun |(spawn-sleep))
(test (ev/call fun))
(test (ev/go |(ev/do-thread (fun))))
)
@bakpakin
Copy link
Member

This is unfortunate behavior relating to closure capture. When you wrap the (test ...) code in the do form, and then create a local binding fun around |(spawn-sleep), you are accidentally capturing too much state with the ev/do-thread macro and that results in the above error.

I don't know if this is easy to fix, but I can look into improving closure capture to not capture the parent fiber.

Note that this also goes away if you use spawn-sleep directly, or move (def fun |(spawn-sleep)) out of the do block.

@bakpakin bakpakin self-assigned this Nov 10, 2023
bakpakin added a commit that referenced this issue Nov 10, 2023
This allows uses the precise closure state capture
when marshalling data between threads. This prevents
accidental state capture when using ev/do-thread or similar
with closures that reference the current state.
bakpakin added a commit that referenced this issue Nov 15, 2023
Another optimization would be to keep track of immutable closure
captures (vs. mutable closure captures) and always detach them.
@bakpakin
Copy link
Member

I'm going to mark this as fixed for now, but I still think there are some improvements to be made here. While the fixes above prevent this kind of error from happening in the case of this macro, we still could reduce variable capture from closures even more than we do already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants