Write files atomically: don't act on signals.

[KuabeM]

What does this PR accomplish?

• 🩹 Bug Fix/Feature

Closes #214 .

Changes proposed by this PR:

Use an AtomicBool to check if writing to disk is currently in progress. Wait for write to be finished and then act on signal as before.

Notes to reviewer:

I thought of surrounding write_changes_to_disk() (action/mod.rs:333). This would be more robust in the future if the match in mod.rs:263 called something else then correct_files(). However, I went for the smaller scope to not ignore signals for too long.

The signal handler is just busy-waiting until the write has been finished.

📜 Checklist

• Works on the ./demo sub directory
• Test coverage is excellent and passes
• Documentation is thorough

[drahnr]

Approach looks good! We should definitely add a testcase for this, even just a syntetic one to check the mechanics used without anything spellcheck specific and excute that with 100 or 1000 iterations

[KuabeM]

Approach looks good! We should definitely add a testcase for this, even just a syntetic one to check the mechanics used without anything spellcheck specific and excute that with 100 or 1000 iterations

I agree. Would you rather fake the SIG or is it enough to 'reimplement' the signal-handler and just test the atomic approach?

[drahnr]

Approach looks good! We should definitely add a testcase for this, even just a syntetic one to check the mechanics used without anything spellcheck specific and excute that with 100 or 1000 iterations

I agree. Would you rather fake the SIG or is it enough to 'reimplement' the signal-handler and just test the atomic approach?

Either is fine by me.

[KuabeM]

Raising signals to self can be done via a raise() syscall, one way would be to use https://docs.rs/syscalls/0.5.0/syscalls/macro.syscall.html. How do you feel about adding such a dependency? Or do you see a different approach?

I guess I'd rather just test the AtomicBool and not the signal handler itself.

[drahnr]

Raising signals to self can be done via a raise() syscall, one way would be to use https://docs.rs/syscalls/0.5.0/syscalls/macro.syscall.html. How do you feel about adding such a dependency? Or do you see a different approach?

It would only be for dev-dependencies right? So that'd be fine

I guess I'd rather just test the AtomicBool and not the signal handler itself.

Without the signal interaction, it's rather pointless :]

[KuabeM]

I'm a bit sick these days, I'll be back on it soon :)

[KuabeM]

Unfortunately, it's not as easy as I thought. cargo seems to install its own signal handler which always just terminates the test.

unning 1 test
error: test failed, to rerun pass '--bin cargo-spellcheck'

Caused by:
  process didn't exit successfully: `/home/korbinian/RustProjects/cargo-spellcheck/target/debug/deps/cargo_spellcheck-d23a42f4a693c8d9 atomic` (signal: 2, SIGINT: terminal interrupt signal)

If I run the above test manually, it seems to work though:

# target/debug/deps/cargo_spellcheck-d23a42f4a693c8d9 atomic

running 1 test
# echo $?
130

Do you have any idea how to circumvent this? Or a different approach how to test this?

[drahnr]

Unfortunately, it's not as easy as I thought. cargo seems to install its own signal handler which always just terminates the test.

unning 1 test
error: test failed, to rerun pass '--bin cargo-spellcheck'

Caused by:
  process didn't exit successfully: `/home/korbinian/RustProjects/cargo-spellcheck/target/debug/deps/cargo_spellcheck-d23a42f4a693c8d9 atomic` (signal: 2, SIGINT: terminal interrupt signal)

If I run the above test manually, it seems to work though:

# target/debug/deps/cargo_spellcheck-d23a42f4a693c8d9 atomic

running 1 test
# echo $?
130

Do you have any idea how to circumvent this? Or a different approach how to test this?

I kind of anticipated this happening 😨

Adding a binary target that is guarded by a test-signal-handing feature, and running that as part of CI would be ok.
For this to work, one first needs to split cargo-spellcheck into a lib and bin target, which is alright, the lib target can just a expose a single fn main function that is then called by lib, plus whatever you need for your testcase.

[KuabeM]

This is a first draft, just moved main.rs to lib.rs and created an ignored integration test which we can then run manually on ci. Will do that next.

Any comments on the main.rs? E.g., documentation wise etc.

[drahnr]

Gentle ping 🙃

[KuabeM]

Finally found some time. I added a binary which is run manually by concourse and exits with 130. If the signal is not properly caught it panics, i.e. exit code 101.

I added the run to concourse for the pr-checks. If the test is fine in general I'll add it to master as well.

Let me know what you're thinking, especially about the main.rs-lib.rs split :)

[drahnr]

A few small nits. I'll do some poking in a local checkout, generally looks good!

[drahnr]

After looking into it for a bit, it seems best to call fork from the integration test as run by rust, you then have the PID of the child process rather the PID of the current process, which consumes the signal itself. So that should then work just fine.

[drahnr]

Changed a few small things, so it can run as regular integration test without the additional setup or CI chores :)