non-parallel mode broken on master

[RReverser]

Looks like 4ef9208 landed yesterday and removes the non-parallel mode added in #194.

These conditional compilation attributes and a separate synchronous code are important for environments like WebAssembly (which is what they were added for in the first place), where thread::spawn is not available and errors out at runtime.

After this change, it's invoked unconditionally again.

[shssoichiro]

Ah, I see. Unfortunately without this change it was failing to compile (i.e. it was already broken). Looks like I'll need to add a WASM target to the CI as well to catch issues like this.

[RReverser]

Hmm, do you know what exactly was broken?

It worked for me after the last PR (#208) and it even powers PNG compression on https://squoosh.app since 2 days ago as mentioned on #203. (Btw, I published a write up about this yesterday in case you're curious: https://rreverser.com/bringing-oxipng-to-squoosh/.)

Maybe something else changed in between within the last week?

[shssoichiro]

I haven't determined what caused this yet, but if I revert 4ef9208, I get these errors:

  cargo clippy --no-default-features
    Checking oxipng v2.3.0 (/home/soichiro/repos/oxipng)
warning: unused import: `std::thread`
  --> src/evaluate.rs:23:5
   |
23 | use std::thread;
   |     ^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

error[E0507]: cannot move out of `self.eval_best_candidate.0`, as `self` is a captured variable in an `FnMut` closure
   --> src/evaluate.rs:154:31
    |
104 |     fn try_image_inner(&self, image: Arc<PngImage>, is_reduction: bool) {
    |                        ----- captured outer variable
...
154 |                         match self.eval_best_candidate {
    |                               ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider borrowing here: `&self.eval_best_candidate`
155 |                             Some(prev) if prev.cmp_key() < new.cmp_key() => {}
    |                                  ----
    |                                  |
    |                                  data moved here
    |                                  move occurs because `prev` has type `evaluate::Candidate`, which does not implement the `Copy` trait

error[E0594]: cannot assign to `self.eval_best_candidate` which is behind a `&` reference
   --> src/evaluate.rs:156:34
    |
156 | ...                   _ => self.eval_best_candidate = Some(new),
    |                            ^^^^^^^^^^^^^^^^^^^^^^^^ cannot assign

error: aborting due to 2 previous errors

I did attempt to resolve it by leaving the feature-dependent code in place and propagating &mut references up the chain, but that did not make the borrow checker very happy.

[RReverser]

Ohh I see what's going on. I was wrong, I didn't actually update to #208 yet and that's the one that broke non-parallel mode. Let me revert 4ef9208 and try to fix this properly instead...

[RReverser]

So the issue is that in #194 I've added RefCell to correctly handle internal mutability, but in #208 I've accidentally removed it and replaced with regular contents.

I just need to add that RefCell back. But yeah, having a CI run at least to build in --no-default-features mode would've been helpful to catch these easier in future.

[shssoichiro]

We do have that now, although it doesn't catch the lack of std::thread on WASM target. But unfortunately for a few weeks Travis had been broken and failing to report results. So it wasn't until a few days ago I migrated the project to Github Actions and we have a working CI again.

[RReverser]

although it doesn't catch the lack of std::thread on WASM target

Yeah that one is trickier to catch. Rust includes the API regardless, but throws a runtime error, so the only way to catch it is by actually running tests (e.g. by compiling to wasm32-wasi and using Node.js- or wasmtime-based runner or something similar).

[RReverser]

Done: #216