[incremental] Don't panic if decoding the cache fails #49364

wesleywiser · 2018-03-25T17:46:27Z

If the cached data can't be loaded from disk, just issue a warning to
the user so they know why compilation is taking longer than usual but
don't fail the entire compilation since we can recover by ignorning the
on disk cache.

In the same way, if the disk cache can't be deserialized (because it has
been corrupted for some reason), report the issue as a warning and
continue without failing the compilation. Decodable::decode() tends to
panic with various errors like "entered unreachable code" or "index out
of range" if the input data is corrupted. Work around this by catching
panics from the decode() calls and continuing without the cached data.

Fixes #48847

rust-highfive · 2018-03-25T17:46:30Z

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

cramertj · 2018-03-25T19:33:32Z

Not sure who to give this to, but definitely not me, so r? @michaelwoerister

Mark-Simulacrum · 2018-03-25T21:01:50Z

src/librustc_incremental/persist/load.rs

+
+    MaybeAsync::Async(std::thread::spawn(move || {
+        time_ext(time_passes, None, "background load prev dep-graph", move || {
+            panic::catch_unwind(load).unwrap_or_else(


Do we need to catch the unwind? Can't we just handle this on thread.join() since we're paying that cost anyway?

That's a good idea! I will do some testing and see if that works.

Yes, that's a good suggestion.

oli-obk · 2018-03-26T05:45:51Z

src/librustc_incremental/persist/load.rs

+    MaybeAsync::Async(std::thread::spawn(move || {
+        time_ext(time_passes, None, "background load prev dep-graph", move || {
+            panic::catch_unwind(load).unwrap_or_else(
+                |_| LoadResult::Error { message: "could not decode incremental cache".into() })


Please emit the actual panic via a debug! or even info! call, so we can figure out what's going on without needing to rebuild the compiler.

I couldn't figure out a way to do this. I tried doing formatting the error into the message with format!("could not decode incremental cache: {:?}", e) but that just printed "could not decode incremental cache: Any".

Maybe someone for @rust-lang/libs knows how to do this best?

There's currently no easy way to get out the message of a Box<Any> unfortunately, but we also tend to recommend against using catch_unwind for error handling like this and would instead recommend returning an Err all the way up (which can then be printed)

The panic handler for the thread is still running so the compiler output looks like this:

Compiling webrender v0.57.0 (file:///Users/wesley/code/rust/temp3/webrender/webrender) thread '<unnamed>' panicked at 'internal error: entered unreachable code', librustc/dep_graph/dep_node.rs:124:34 note: Run with `RUST_BACKTRACE=1` for a backtrace. warning: could not decode incremental cache

michaelwoerister · 2018-03-26T09:29:49Z

Thanks for the PR, @wesleywiser! I'll take a look shortly.

michaelwoerister · 2018-03-28T10:00:41Z

Thanks, @wesleywiser!

@bors r+

bors · 2018-03-28T10:00:42Z

📌 Commit 369f34c has been approved by michaelwoerister

If the cached data can't be loaded from disk, just issue a warning to the user so they know why compilation is taking longer than usual but don't fail the entire compilation since we can recover by ignorning the on disk cache. In the same way, if the disk cache can't be deserialized (because it has been corrupted for some reason), report the issue as a warning and continue without failing the compilation. `Decodable::decode()` tends to panic with various errors like "entered unreachable code" or "index out of range" if the input data is corrupted. Work around this by catching panics from the `decode()` calls when joining the thread and continuing without the cached data. Fixes rust-lang#48847

wesleywiser · 2018-03-28T12:31:20Z

Rebased

michaelwoerister · 2018-03-28T12:34:23Z

@bors r+

bors · 2018-03-28T12:34:24Z

📌 Commit 49fd71b has been approved by michaelwoerister

…e, r=michaelwoerister [incremental] Don't panic if decoding the cache fails If the cached data can't be loaded from disk, just issue a warning to the user so they know why compilation is taking longer than usual but don't fail the entire compilation since we can recover by ignorning the on disk cache. In the same way, if the disk cache can't be deserialized (because it has been corrupted for some reason), report the issue as a warning and continue without failing the compilation. `Decodable::decode()` tends to panic with various errors like "entered unreachable code" or "index out of range" if the input data is corrupted. Work around this by catching panics from the `decode()` calls and continuing without the cached data. Fixes rust-lang#48847

Rollup of 12 pull requests - Successful merges: #49243, #49329, #49364, #49400, #49405, #49427, #49428, #49429, #49439, #49442, #49444, #49452 - Failed merges:

jsgf · 2018-03-29T00:44:21Z

Thanks for fixing this!

rust-highfive assigned cramertj Mar 25, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 25, 2018

rust-highfive assigned michaelwoerister and unassigned cramertj Mar 25, 2018

Mark-Simulacrum reviewed Mar 25, 2018

View reviewed changes

oli-obk reviewed Mar 26, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2018

wesleywiser force-pushed the incr_handle_load_failure branch from 369f34c to 49fd71b Compare March 28, 2018 12:31

kennytm mentioned this pull request Mar 28, 2018

Rollup of 12 pull requests #49460

Merged

bors added a commit that referenced this pull request Mar 28, 2018

Auto merge of #49460 - kennytm:rollup, r=kennytm

d52c44e

Rollup of 12 pull requests - Successful merges: #49243, #49329, #49364, #49400, #49405, #49427, #49428, #49429, #49439, #49442, #49444, #49452 - Failed merges:

bors merged commit 49fd71b into rust-lang:master Mar 29, 2018

imp mentioned this pull request Apr 2, 2018

rustc is still failing with "index out of bounds" when incremental is on #49586

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[incremental] Don't panic if decoding the cache fails #49364

[incremental] Don't panic if decoding the cache fails #49364

wesleywiser commented Mar 25, 2018

rust-highfive commented Mar 25, 2018

cramertj commented Mar 25, 2018

Mark-Simulacrum Mar 25, 2018

wesleywiser Mar 26, 2018

michaelwoerister Mar 27, 2018

oli-obk Mar 26, 2018

wesleywiser Mar 26, 2018

michaelwoerister Mar 27, 2018

alexcrichton Mar 27, 2018

wesleywiser Mar 28, 2018

michaelwoerister commented Mar 26, 2018

michaelwoerister commented Mar 28, 2018

bors commented Mar 28, 2018

wesleywiser commented Mar 28, 2018

michaelwoerister commented Mar 28, 2018

bors commented Mar 28, 2018

jsgf commented Mar 29, 2018

[incremental] Don't panic if decoding the cache fails #49364

[incremental] Don't panic if decoding the cache fails #49364

Conversation

wesleywiser commented Mar 25, 2018

rust-highfive commented Mar 25, 2018

cramertj commented Mar 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelwoerister commented Mar 26, 2018

michaelwoerister commented Mar 28, 2018

bors commented Mar 28, 2018

wesleywiser commented Mar 28, 2018

michaelwoerister commented Mar 28, 2018

bors commented Mar 28, 2018

jsgf commented Mar 29, 2018