#development
Title
h
hundreds-breakfast-49010
10/01/2019, 10:52 PMI'm trying to fix https://github.com/pantsbuild/pants/pull/8329#discussion_r328735829 and I'm not how to go about constructing a rust FFI function that avoids calling
scheduler.core.executor.block_onPyResulta
average-vr-56795
10/01/2019, 10:54 PMI suspect you’ll need to turn it into something that you can
yieldGraphh
hundreds-breakfast-49010
10/01/2019, 10:55 PMthat makes sense
are we doing this anywhere else in the codebase currently?
I'm not sure how python yield semantics interact with rust-side future semantics
a
average-vr-56795
10/01/2019, 11:06 PMOnly for things in the graph, currently…
@witty-crayon-22786 ^^
w
witty-crayon-22786
10/02/2019, 2:19 AMsoooo
hmmmm
lemme respond to this in a bit. will take some thinking.
hm. yea, dang.
while we could hypothetically just block this thread in some other way that won't cause the tokio executor to complain, we would still be blocking one of the tokio runtime's functions.
now, @average-vr-56795 actually explored an alternative to blocking threads on this IO at one point, and i think that if we were to switch back to that workflow we would be ok.
https://github.com/pantsbuild/pants/pull/7788 was one of the tickets... and i fully expect that at some point switching back to that mode will be feasible.
in the meantime, i think that if we add another copy of the relevant
materializeruntime.block_onFuturei say "ok", because, if we ensured that this method was only called from a
@console_ruleand if we set a minimum number of threads for the tokio runtime, blocking one thread should never cause a deadlock.
so: we'd attach a caveat to that, and we'd reference switching back to non-blocking IO for this codepath (@average-vr-56795: did we end up with a ticket for trying that again?)
h
hundreds-breakfast-49010
10/02/2019, 7:49 PMdid you see john's suggestion on https://github.com/pantsbuild/pants/pull/8329#discussion_r330613798 ?
w
witty-crayon-22786
10/02/2019, 8:00 PMi did not, sorry. lots o email. thanks
@hundreds-breakfast-49010: commented.
h
hundreds-breakfast-49010
10/02/2019, 10:50 PMjumping back to this. I'm actually confused about why the call to
block_onmaterialize_directoriesand that
block_ontokio::runtime::Runtimeblock_onI don't know why creating a separate runtime causes this panic
(also when I actually run my test I don't get a rust panic, I get a segfault - maybe that's the expected behavior if a rust panic happens inside a CFFI function?)
(https://doc.rust-lang.org/std/panic/fn.catch_unwind.html, "It is currently undefined behavior to unwind from Rust code into foreign code, so this function is particularly useful when Rust is called from another language (normally C). This can run arbitrary Rust code, capturing a panic and allowing a graceful handling of the error." . so maybe we should be wrapping all our invocations of rust functions called from python in a
catch_unwindbut back to the runtime question, I'm not sure why a blocking operation on one tokio
RuntimeRuntimeI also notice that that
block_on&selftask_executor::Executor&selfw
witty-crayon-22786
10/02/2019, 11:19 PMcatch_unwind is... also ill defined, i thought
my expectation is that we are not using a new anonymous instance, but that that Runtime call uses a thread local instance of the runtime
if we were using a new one each time we'd be creating new pools
h
hundreds-breakfast-49010
10/02/2019, 11:22 PMhttps://docs.rs/tokio/0.1.22/tokio/runtime/struct.Runtime.html#method.new is the documentation for
Runtime::newit doesn't say anything about using a thread local already existing instance of a runtime
but I'm not very familiar with tokio so I might be misunderstanding what's going on
w
witty-crayon-22786
10/02/2019, 11:25 PMwell... that's.. awkward. heh.
there are a lot of possibilities: this version of the method could launch the work on the
CpuPoolfuture.wait()h
hundreds-breakfast-49010
10/02/2019, 11:27 PMso that's the
io_poolExecutorw
witty-crayon-22786
10/02/2019, 11:27 PMoff topic, i suppose, but: we should probably not be creating new
Runtimesyep
with a big fat comment explaining the above
h
hundreds-breakfast-49010
10/02/2019, 11:28 PMso does that imply that the original implementation of
materialize_directoriesblock_onw
witty-crayon-22786
10/02/2019, 11:28 PMnot wrong, just possibly less efficient.
we should be using ~one runtime (maybe there is a comment somewhere nearby that explains why we don't)
h
hundreds-breakfast-49010
10/02/2019, 11:29 PManyway, it looks like I can call
spawnI'm not sure how cpu pools interact with tokio runtimes
w
witty-crayon-22786
10/02/2019, 11:29 PMthey mostly don't: the tokio runtime is one threadpool, the cpupool is another.
h
hundreds-breakfast-49010
10/02/2019, 11:30 PMbut then I have a
CpuFuturew
witty-crayon-22786
10/02/2019, 11:30 PMwhen you call `future.wait()`on the thing returned by the CpuPool, you will be doing so on the tokio runtime, and blocking one of its threads
h
hundreds-breakfast-49010
10/02/2019, 11:30 PMthere's no way around forcing a future to complete and yield a
ResultPyResultw
witty-crayon-22786
10/02/2019, 11:30 PMfuture.wait()h
hundreds-breakfast-49010
10/02/2019, 11:31 PMw
witty-crayon-22786
10/02/2019, 11:31 PMyes. we need to block the thread. see above.
h
hundreds-breakfast-49010
10/02/2019, 11:32 PMoh
Futurew
witty-crayon-22786
10/02/2019, 11:32 PMyea. you'll call wait on the one returned by the CpuPool
er. and spawn doesn't give you one directly... so maybe you want spawn_fn? can't remember
h
hundreds-breakfast-49010
10/02/2019, 11:33 PMExecutorio_poolwhich I could change to pub I guess, but maybe there's a reason it's not exposed right now
w
witty-crayon-22786
10/02/2019, 11:37 PMit exposes it ... somehow, yea?
h
hundreds-breakfast-49010
10/02/2019, 11:41 PMokay, I think doing this gets my test to no longer segfault
woo
w
witty-crayon-22786
10/02/2019, 11:43 PMwoot 😃
h
hundreds-breakfast-49010
10/03/2019, 12:11 AMupdated: https://github.com/pantsbuild/pants/pull/8329 @average-vr-56795