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

There is a problem with the lock file lock in runner. #270

Closed
zong-zhe opened this issue Oct 31, 2022 · 6 comments
Closed

There is a problem with the lock file lock in runner. #270

zong-zhe opened this issue Oct 31, 2022 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@zong-zhe
Copy link
Contributor

zong-zhe commented Oct 31, 2022

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

File lock only locks 'main'.

2. What did you expect to see? (Required)

File locks should lock all 'pkgpath'

3. What did you see instead (Required)

running 1 test
thread '<unnamed>' panicked at 'Compile KCL to LLVM error: "No such file or directory"', runner/src/assembler.rs:164:10
stack backtrace:
   0: rust_begin_unwind
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/panicking.rs:142:14
   2: core::result::unwrap_failed
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/result.rs:1814:5
   3: core::result::Result<T,E>::expect
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/result.rs:1064:23
   4: <kclvm_runner::assembler::LlvmLibAssembler as kclvm_runner::assembler::LibAssembler>::assemble
             at ./src/assembler.rs:155:9
   5: <kclvm_runner::assembler::KclvmLibAssembler as kclvm_runner::assembler::LibAssembler>::assemble
             at ./src/assembler.rs:97:40
   6: kclvm_runner::assembler::KclvmAssembler::gen_libs::{{closure}}
             at ./src/assembler.rs:353:45
   7: <F as threadpool::FnBox>::call_box
             at /Users/.cargo/registry/src/github.com-1ecc6299db9ec823/threadpool-1.8.1/src/lib.rs:95:9
   8: threadpool::spawn_in_pool::{{closure}}
             at /Users/.cargo/registry/src/github.com-1ecc6299db9ec823/threadpool-1.8.1/src/lib.rs:769:17
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

4. What is your KusionStack components version? (Required)

@zong-zhe zong-zhe added the bug Something isn't working label Oct 31, 2022
@zong-zhe zong-zhe added this to the v0.4.4 Release milestone Oct 31, 2022
@zong-zhe zong-zhe self-assigned this Oct 31, 2022
@zong-zhe
Copy link
Contributor Author

@zong-zhe
Copy link
Contributor Author

zong-zhe commented Oct 31, 2022

After PR #71 and #215, this bug appeared, there is a problem with file locking during concurrent compilation.

Before PR #71 and #215, the main package and non-main package will lock the different files, compiling main package will lock a temp file whose filename based on time and process id and compiling other package will lock the file whose filename is '<package_name>.ll.lock'.

The file lock used in concurrent compilation will lock the file:
code from 8d20c6a before PR #71 and #215.

// kclvm/src/lib.rs/kclvm_cli_run_unsafe
let file = if is_main_pkg {
    // Use temp main entry to prevent conflicts between different mains.

    // Compiling 'main.k' will lock a temporary file.
    PathBuf::from(&temp_entry_file)

} else {

    // Compiling other package files will lock the 'package_filepath.ll.lock'.lock.
    cache_dir.join(&pkgpath)

};
let ll_file = file.to_str().unwrap();
let ll_path = format!("{}.ll", ll_file);
let dylib_path = format!("{}{}", ll_file, Command::get_lib_suffix());
let mut ll_path_lock = fslock::LockFile::open(&format!("{}.lock", ll_path)).unwrap();
ll_path_lock.lock().unwrap();

@zong-zhe
Copy link
Contributor Author

zong-zhe commented Oct 31, 2022

After PR #71 and #215, both compiling main package and non-main package will lock a temp file whose filename based on time and process id.

For the non-main package, when multiple threads access the same file, they will acquire different file locks, which will cause the lock to fail. Multiple threads should acquire the same file lock when accessing the same file.

The file lock used in concurrent compilation will lock the file:
code from 05def8d

// kclvm/runner/src/assembler.rs/impl KclvmAssembler/gen_libs

  // `self.entry_file` will generate different file locks for the same file.
  let code_file = self.entry_file.clone();
  let code_file_path = assembler.add_code_file_suffix(&code_file);
  let lock_file_path = format!("{}.lock", code_file_path);
  let lib_path = format!("{}{}", code_file, Command::get_lib_suffix());

self.entry_file comes from the struct KclvmAssembler.

// kclvm/runner/src/assembler.rs/KclvmAssembler
pub(crate) struct KclvmAssembler {
    thread_count: usize,
    program: ast::Program,
    scope: ProgramScope,
    entry_file: String,
    single_file_assembler: KclvmLibAssembler,
}

self.entry_file is initialized at below:

// kclvm/runner/src/lib.rs
// Create a temp entry file and the temp dir will be delete automatically
let temp_dir = tempdir().unwrap();
let temp_dir_path = temp_dir.path().to_str().unwrap();
let temp_entry_file = temp_file(temp_dir_path);

// Generate libs
let lib_paths = assembler::KclvmAssembler::new(
    program,
    scope,
    temp_entry_file.clone(),
    KclvmLibAssembler::LLVM,
)
.gen_libs();

The method temp_file() will generate a file based on time and process_id.

@ldxdl
Copy link
Contributor

ldxdl commented Oct 31, 2022

What is the solution to the problem?
How can we be sure to prevent the issue from happening again?

@zong-zhe
Copy link
Contributor Author

PR #272 fixed the bug, and created different file locks according to the current package type before concurrent compilation.

code from feca470:

// kclvm/runner/src/assembler.rs/impl KclvmAssembler/gen_libs

let entry_file = self.entry_file.clone();
let assembler = self.single_file_assembler.clone();
let is_main_pkg = pkgpath == kclvm_ast::MAIN_PKG;

// Created different file locks according to the current package type before concurrent compilation
let file = if is_main_pkg {
    PathBuf::from(entry_file) // Main package 
} else {
    cache_dir.join(&pkgpath) // Non-main package 
};
let code_file = file.to_str().unwrap().to_string();
... ...

// Concurrent compilation begin.
pool.execute(move || {
     ... ...
})

@zong-zhe
Copy link
Contributor Author

What is the solution to the problem? How can we be sure to prevent the issue from happening again?

I will add some more test cases for concurrent compilation to expose the problems as much as possible.

ldxdl pushed a commit that referenced this issue Nov 7, 2022
#272)

* Fix(kclvm-runner): Added different file locks depending on the currently package.

If the current package is the main package, the lock will lock a temporary file based on time and process id.
If the current package is not the main package, the lock will lock a file named 'package_path.ll.lock'.

issue #270.

* rm useless comments

* add a large scale test case

* add konfig test

* add some comments
@Peefy Peefy closed this as completed Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants