janet icon indicating copy to clipboard operation
janet copied to clipboard

peg-grammar is not available in threads

Open zevv opened this issue 1 year ago • 5 comments

This snippet:

(ev/spawn-thread       
  (peg/match '{ :main :s* } "")) 

results in the following error:

error: grammar error in :s*, unknown rule
  in peg/match [src/core/peg.c] on line 1694
  in _spawn-thread [/tmp/t.janet] on line 2, column 3

Root cause is that the *peg-grammar* dynamic binding does not make it into the thread, just like any other dynamic binding.

zevv avatar Jun 04 '23 10:06 zevv

Yes, although not reported in an issue AFAIK, this has been noted in discussions at one or more of the gitter / matrix channels.

A work-around I'd been using was putting:

(setdyn :peg-grammar default-peg-grammar)

in the definition of a function passed to ev/thread.

It seems to me that either it should be documented behavior (along with any other caveats [1] when spawning a thread) or the current behavior should be adjusted.


[1] I noticed that eval didn't give quite the same results in my testing, and I suppose it may be a related thing.

sogaiu avatar Jun 04 '23 11:06 sogaiu

I must admit I have not looked deeply at the memory model for threads so I'm not sure what the original design decisions are, but I think conceptually it might make sense to have new threads inherit the parent's dynamic bindings?

zevv avatar Jun 04 '23 13:06 zevv

For coro, the fiber that's created with fiber/new has :yi as part of its sigmask, and :i is "inherit the environment from the current fiber".

So if I do:

(ev/thread (coro (pp (peg/match {:main :s*} ""))))

I get:

@[]
nil

Currently ev/spawn-thread and ev/do-thread don't take any flags, but may be it would be nice if they did? Something like:

(ev/spawn-thread :i (peg/match {:main :s*} ""))

I don't suppose that would break any code -- an initial keyword in a function body presumably doesn't do much so I doubt many people have been using them for anything "usual" [1].


[1] I've seen use as what looks like labeling (presumably for human readers) as well as metadata that gets parsed by other code.

sogaiu avatar Jun 04 '23 14:06 sogaiu

So the ev/thread function, which is what does all of the heavy lifting of setting up a new thread, takes some flags to determine what state to copy from the current thread into the new thread. I suppose it would make sense to copy over dynamic bindings as an option if specified - however, copying over the entire environment table as returned by (curenv), table proto types and all, is prohibitively expensive.

I noticed that eval didn't give quite the same results in my testing, and I suppose it may be a related thing.

Yes, eval needs to have available function definitions in it's environment, so in order to see all available functions they would need to all be copied to the new thread.

For efficiency, the default behavior is to copy nothing that isn't needed.

bakpakin avatar Jun 04 '23 16:06 bakpakin

For completeness, as a work around I should mention that ev/thread can take a fiber argument as well as a function argument - while this doesn't help if you are using the built-in ev/do-thread and ev/spawn-thread macros, you could create your own subroutine to wrap a closure or function body in a fiber that captures all of the state you were interested in.

For example

(defn wrap-closure-for-thread
  "Wrap a closure in a fiber that captures the current dynamic bindings state."
  [f]
  (def env @{})
  (each k (all-dynamics) (put env k (dyn k)))
  (fiber/setenv (fiber/new f :t) env))
  
# Use with ev/thread
(ev/thread (wrap-closure-for-thread (fn [&] (pp (dyn *peg-grammar*)))))

Edit: @sogaiu already mentioned this but above is more explicit about what is happening and how to capture precisely the state that you want.

bakpakin avatar Jun 04 '23 19:06 bakpakin