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

cbindgen panicks when parse-expand is run inside of buildscript due to inability to acquire semaphore #347

Closed
Wodann opened this issue May 23, 2019 · 5 comments

Comments

@Wodann
Copy link
Contributor

Wodann commented May 23, 2019

I have successfully run cbingen on the command-line using the following cbindgen.toml:

include_guard = "SE4_CORE_BINDINGS_H_"
include_version = true
braces = "SameLine"
line_length = 100
tab_width = 4

[export]
prefix = "SE4"

[parse]
parse_deps = false

[parse.expand]
crates = ["se4_capi"]

I decided to add it as a build script:

extern crate cbindgen;

use std::env;

fn main() {
    let crate_dir = env::var("CARGO_MANIFEST_DIR").unwrap();

    cbindgen::generate(crate_dir)
        .expect("Unable to generate bindings")
        .write_to_file("include/bindings.h");
}

Upon execution, of cargo build cbindgen instantly crashes. It seems like cbindgen tries to acquire a lock on the build directory that was previously acquired by cargo, and thus crashes after outputing the following:

    Blocking waiting for file lock on build directory
   Compiling se4_capi v0.1.0
error: failed to run custom build command for `se4_capi v0.1.0`

Caused by:
  process didn't exit successfully: `build-script-build` (exit code: 101)
--- stderr
thread 'main' panicked at 'Unable to generate bindings: CargoExpand("se4_capi", Compile("    Blocking waiting for file lock on build directory\nerror: Too many posts were made to a semaphore. (os error 298)\n"))', src\libcore\result.rs:999:5
stack backtrace:
   0: std::sys::windows::backtrace::set_frames
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\/src\libstd\sys\windows\backtrace\mod.rs:95
   1: std::sys::windows::backtrace::unwind_backtrace
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\/src\libstd\sys\windows\backtrace\mod.rs:82
   2: std::sys_common::backtrace::_print
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\/src\libstd\sys_common\backtrace.rs:71
   3: std::sys_common::backtrace::print
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\/src\libstd\sys_common\backtrace.rs:59
   4: std::panicking::default_hook::{{closure}}
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\/src\libstd\panicking.rs:197
   5: std::panicking::default_hook
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\/src\libstd\panicking.rs:211
   6: std::panicking::rust_panic_with_hook
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\/src\libstd\panicking.rs:474
   7: std::panicking::continue_panic_fmt
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\/src\libstd\panicking.rs:381
   8: std::panicking::rust_begin_panic
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\/src\libstd\panicking.rs:308
   9: core::panicking::panic_fmt
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\/src\libcore\panicking.rs:85
  10: core::result::unwrap_failed<cbindgen::bindgen::error::Error>
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\src\libcore\macros.rs:18
  11: core::result::Result<cbindgen::bindgen::bindings::Bindings, cbindgen::bindgen::error::Error>::expect<cbindgen::bindgen::bindings::Bindings,cbindgen::bindgen::error::Error>
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\src\libcore\result.rs:827
  12: build_script_build::main
             at .\build.rs:9
  13: std::rt::lang_start::{{closure}}<()>
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\src\libstd\rt.rs:64
  14: std::rt::lang_start_internal::{{closure}}
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\/src\libstd\rt.rs:49
  15: std::panicking::try::do_call<closure,i32>
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\/src\libstd\panicking.rs:293
  16: panic_unwind::__rust_maybe_catch_panic
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\/src\libpanic_unwind\lib.rs:85
  17: std::panicking::try
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\/src\libstd\panicking.rs:272
  18: std::panic::catch_unwind
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\/src\libstd\panic.rs:388
  19: std::rt::lang_start_internal
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\/src\libstd\rt.rs:48
  20: std::rt::lang_start<()>
             at /rustc/37ff5d388f8c004ca248adb635f1cc84d347eda0\src\libstd\rt.rs:64
  21: main
  22: invoke_main
             at d:\agent\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:78
  23: __scrt_common_main_seh
             at d:\agent\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
  24: BaseThreadInitThunk
  25: RtlUserThreadStart

I am aware of PR #72, and as such attempted to set CARGO_EXPAND_TARGET_DIR, but the only change was that it took a little longer for the panick to occur.

env::set_var("CARGO_EXPAND_TARGET_DIR", crate_dir.clone());

I also read PR #153 which seems to have a similar problem, except that it doesn't result in a panick.

To use cbindgen I swapped to nightly, as it was required for expanding macros. I am currently at:

nightly-x86_64-pc-windows-msvc (default)
rustc 1.36.0-nightly (37ff5d388 2019-05-22)
@emilio
Copy link
Collaborator

emilio commented May 23, 2019

Do you know if this is a regression?

@Wodann
Copy link
Contributor Author

Wodann commented May 24, 2019

This was the first time I used cbindgen, so I hounestly wouldn't know.

vmx added a commit to vmx/cbindgen-expand-bug that referenced this issue Jun 28, 2019
I ran into [#347] on a different repository. I now tried
to reproduce it with a minimal example. The problem seems
to have to do with locking files.

Even if you let the expansion happen in a different directory,
there seems to be a locking deadlock.

```console
$ lslocks|grep cargo
cargo           10199  FLOCK       WRITE  0          0          0 /home/vmx/src/rust/cbindgen/hello/target/debug/.carg -lock
cargo           12301  FLOCK       WRITE* 0          0          0 /home/vmx/src/rust/cbindgen/hello/expanded/debug/.cargo-lock
cargo           10204  FLOCK       WRITE  0          0          0 /home/vmx/src/rust/cbindgen/hello/expanded/debug/.cargo-lock
```

Process 12301 tries to get the lock, but can't.

This is easily repoducible with this repo. Just run it like that:

```console
$ CARGO_EXPAND_TARGET_DIR=./expanded cargo build
…
   Compiling cbindgen-expand-bug v0.1.0 (/home/vmx/src/rust/cbindgen/cbindgen-expand-bug)
    Building [====================================================>  ] 101/103: cbindgen-expand-bug(build)
```

This will then start to hang. You can now check the file locks as
outlined above.

[#347]: mozilla/cbindgen#347
vmx added a commit to vmx/cbindgen-expand-bug that referenced this issue Jun 28, 2019
I ran into [#347] on a different repository. I now tried
to reproduce it with a minimal example. The problem seems
to have to do with locking files.

Even if you let the expansion happen in a different directory,
there seems to be a locking deadlock.

```console
$ lslocks|grep cargo
cargo           10199  FLOCK       WRITE  0          0          0 /home/vmx/src/rust/cbindgen/hello/target/debug/.carg -lock
cargo           12301  FLOCK       WRITE* 0          0          0 /home/vmx/src/rust/cbindgen/hello/expanded/debug/.cargo-lock
cargo           10204  FLOCK       WRITE  0          0          0 /home/vmx/src/rust/cbindgen/hello/expanded/debug/.cargo-lock
```

Process 12301 tries to get the lock, but can't.

This is easily repoducible with this repo. Just run it like that:

```console
$ CARGO_EXPAND_TARGET_DIR=./expanded cargo build
…
   Compiling cbindgen-expand-bug v0.1.0 (/home/vmx/src/rust/cbindgen/cbindgen-expand-bug)
    Building [====================================================>  ] 101/103: cbindgen-expand-bug(build)
```

You can now start to watch the locks:

```console
$ watch 'lslocks|grep cargo
Every 2.0s: lslocks|grep cargo                                                           gene: Fri Jun 28 17:16:15 2019

cargo           29961  FLOCK       WRITE* 0          0          0 /tmp/cbindgen-expand-bug/expanded/debug/.cargo-lock
cargo           27746  FLOCK       WRITE  0          0          0 /tmp/cbindgen-expand-bug/expanded/debug/.cargo-lock
cargo           25472  FLOCK       WRITE  0          0          0 /tmp/cbindgen-expand-bug/target/debug/.cargo-lock
```

In the beginning you will only see a `WRITE` lock on the `target`
directory. Next will be the lock on `expanded` and finally a
second `WRITE` lock will appear on expanded.

[#347]: mozilla/cbindgen#347
@vmx
Copy link
Contributor

vmx commented Jun 28, 2019

I also ran into this issue. I created a minimal example for this problem at https://github.com/vmx/cbindgen-expand-bug. I hope it helps fixing the bug. Here's the summary about what I found out:

I now tried to reproduce it with a minimal example. The problem seems to have to do with locking files.

Even if you let the expansion happen in a different directory, there seems to be a locking deadlock.

$ lslocks|grep cargo
cargo           10199  FLOCK       WRITE  0          0          0 /home/vmx/src/rust/cbindgen/hello/target/debug/.carg -lock
cargo           12301  FLOCK       WRITE* 0          0          0 /home/vmx/src/rust/cbindgen/hello/expanded/debug/.cargo-lock
cargo           10204  FLOCK       WRITE  0          0          0 /home/vmx/src/rust/cbindgen/hello/expanded/debug/.cargo-lock

Process 12301 tries to get the lock, but can't.

This is easily repoducible with this repo. Just run it like that:

$ CARGO_EXPAND_TARGET_DIR=./expanded cargo build

   Compiling cbindgen-expand-bug v0.1.0 (/home/vmx/src/rust/cbindgen/cbindgen-expand-bug)
    Building [====================================================>  ] 101/103: cbindgen-expand-bug(build)

You can now start to watch the locks:

$ watch 'lslocks|grep cargo
Every 2.0s: lslocks|grep cargo                                                           gene: Fri Jun 28 17:16:15 2019

cargo           29961  FLOCK       WRITE* 0          0          0 /tmp/cbindgen-expand-bug/expanded/debug/.cargo-lock
cargo           27746  FLOCK       WRITE  0          0          0 /tmp/cbindgen-expand-bug/expanded/debug/.cargo-lock
cargo           25472  FLOCK       WRITE  0          0          0 /tmp/cbindgen-expand-bug/target/debug/.cargo-lock

In the beginning you will only see a WRITE lock on the target directory. Next will be the lock on expanded and finally a second WRITE lock will appear on expanded.

@vmx
Copy link
Contributor

vmx commented Jul 19, 2019

I'm getting closer. The problem is when cbindgen is called from build.rs. When you want to expand such a crate it will call cargo again, which then will call the build.rs again and hence end up in an endless recursion loop.

I tried to fix it like this:

diff --git a/src/bindgen/cargo/cargo_expand.rs b/src/bindgen/cargo/cargo_expand.rs
index 905fc80..5c1a58e 100644
--- a/src/bindgen/cargo/cargo_expand.rs
+++ b/src/bindgen/cargo/cargo_expand.rs
@@ -77,6 +77,10 @@ pub fn expand(
         cmd.env("CARGO_TARGET_DIR", path);
     }
 
+    // Set this variable so that we don't call it recursively if we expand a crate that is using
+    // cbindgen
+    cmd.env("_CBINDGEN_IS_RUNNING", "1");
+
     cmd.arg("rustc");
     cmd.arg("--lib");
     cmd.arg("--manifest-path");
diff --git a/src/bindgen/parser.rs b/src/bindgen/parser.rs
index a7cd618..bfa4c21 100644
--- a/src/bindgen/parser.rs
+++ b/src/bindgen/parser.rs
@@ -179,6 +179,13 @@ impl<'a> Parser<'a> {
     fn parse_expand_crate(&mut self, pkg: &PackageRef) -> Result<(), Error> {
         assert!(self.lib.is_some());
 
+        // If you want to expand the crate you run cbindgen on you might end up in an endless
+        // recursion if the cbindgen generation is triggered from build.rs. Hence don't run the
+        // expansion if the build was already triggered by cbindgen.
+        if std::env::var("_CBINDGEN_IS_RUNNING").is_ok() {
+            return Ok(());
+        }
+
         let mod_parsed = {
             if !self.cache_expanded_crate.contains_key(&pkg.name) {
                 let s = self

This is already pretty close. It works well if you have CARGO_EXPAND_TARGET_DIR defined. It also works well without CARGO_EXPAND_TARGET_DIR if you run the build script manually (if you run cargo build --verbose first).

So if anyone has a better idea on how to make sure that cbindgen isn't running itself again, that would be great.

BTW: It's not a regression I'm able to reproduce it down to version 0.2 easily.

@vmx
Copy link
Contributor

vmx commented Jul 26, 2019

The patch above combined with this patch

--- a/src/bindgen/cargo/cargo_expand.rs
+++ b/src/bindgen/cargo/cargo_expand.rs
@@ -6,7 +6,7 @@ use std::env;
 use std::error;
 use std::fmt;
 use std::io;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::process::Command;
 use std::str::{from_utf8, Utf8Error};
 
@@ -75,6 +75,11 @@ pub fn expand(
         cmd.env("CARGO_TARGET_DIR", _temp_dir.unwrap().path());
     } else if let Ok(ref path) = env::var("CARGO_EXPAND_TARGET_DIR") {
         cmd.env("CARGO_TARGET_DIR", path);
+    } else {
+       // Don't use the default dir, else we could end up in a deadlock cbindgen is run from
+       // a build.rs file
+       let out_dir = PathBuf::from(&env::var("OUT_DIR").unwrap());
+       cmd.env("CARGO_TARGET_DIR", out_dir.join("expanded"));
     }
 
     // Set this variable so that we don't call it recursively if we expand a crate that is using

make the build at least pass. I still need to confirm if it actually does the right thing.

vmx added a commit to vmx/cbindgen that referenced this issue Aug 2, 2019
When crates should get expanded, then cargo is run again from within cbindgen.
When cbindgen is started from a build.rs file, this means that cargo is starting
another cargo run.

Cargo locks the directory it is running on. If two cargos spawn by each other
run with the same target directory, then they both want to accquire a lock and
hence deadlock.

This commit fixes the problem with running the spawned cargo at a different
directory. Please note that this will always be the same directory, hence
any subsequent runs will be faster (as you would exepct it to be).

This is part of mozilla#347.
vmx added a commit to vmx/cbindgen that referenced this issue Aug 2, 2019
To expand a crate, cbindgen calls cargo on that crate. cbindgen can be called from a build.rs
file. But if cargo is called again, it will also call cbindgen again and hence end up in an
endless recursion.

This commit makes sure that cbindgen isn't called again if it is already running.

You can verify this fix with this minimal example https://github.com/vmx/cbindgen-expand-bug

Fixes mozilla#347.
vmx added a commit to vmx/cbindgen that referenced this issue Aug 3, 2019
When crates should get expanded, then cargo is run again from within cbindgen.
When cbindgen is started from a build.rs file, this means that cargo is starting
another cargo run.

Cargo locks the directory it is running on. If two cargos spawn by each other
run with the same target directory, then they both want to accquire a lock and
hence deadlock.

This commit fixes the problem with running the spawned cargo at a different
directory. Please note that this will always be the same directory, hence
any subsequent runs will be faster (as you would exepct it to be).

This is part of mozilla#347.
vmx added a commit to vmx/cbindgen that referenced this issue Aug 3, 2019
To expand a crate, cbindgen calls cargo on that crate. cbindgen can be called from a build.rs
file. But if cargo is called again, it will also call cbindgen again and hence end up in an
endless recursion.

This commit makes sure that cbindgen isn't called again if it is already running.

You can verify this fix with this minimal example https://github.com/vmx/cbindgen-expand-bug

Fixes mozilla#347.
emilio pushed a commit that referenced this issue Aug 6, 2019
When crates should get expanded, then cargo is run again from within cbindgen.
When cbindgen is started from a build.rs file, this means that cargo is starting
another cargo run.

Cargo locks the directory it is running on. If two cargos spawn by each other
run with the same target directory, then they both want to accquire a lock and
hence deadlock.

This commit fixes the problem with running the spawned cargo at a different
directory. Please note that this will always be the same directory, hence
any subsequent runs will be faster (as you would exepct it to be).

This is part of #347.
@emilio emilio closed this as completed in 65958a5 Aug 6, 2019
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

3 participants