Sled: IO error opening database causes panic

[portstrom]

I would expect any operation that can encounters an IO error to return the error, not to panic. The method ConfigBuilder::build however panics on IO errors, even though the underlying function causing the IO errors gracefully returns them. It would be a quick fix to also make the builder return a Result instead of panicking.

Test case 1: Invalid path: sled::ConfigBuilder::new().path("\0").build()

Test case 2: Insufficient permissions: chmod 000 database

https://github.com/spacejam/sled/blob/06ed24bdf606187259df0bdb977987a0a1c312b7/crates/pagecache/src/config.rs#L175

[spacejam]

It's this way because this is not an issue that is usually solvable programmatically. But this is an API that has gone back and forth over time. I haven't had anyone say that they need to do specific recovery logic when this fails yet, rather than panic. Given that it adds API friction for everybody to do so, I haven't done this yet. Do you intend to do this?

[spacejam]

If you were actually blocked due to not being able to handle this, I would consider changing it. But this represents a fairly major misconfiguration, and I assume everyone would unwrap it and crash anyway.

[crusty-dave]

Unrelated to this specific issue, but one of our testers is hitting a panic due to an out-of-memory condition. I am probably going to switch to a different DB at this point if switching from 0.26 to 0.28 doesn't resolve it.

I don't ever expect a rust application to panic if the app is handling exceptions.

[spacejam]

@crusty-dave please open a separate issue and include information that may help resolving the issue for others. It's not clear that your testers are hitting a sled issue or another OOM issue.

[portstrom]

It's this way because this is not an issue that is usually solvable programmatically.

That's not a valid reason. Panics indicate bugs in a system, not things that are not solvable programmatically. Image your web browser panicking when it fails to load a page that doesn't exist, because it can't programmatically make the page exist.

I haven't had anyone say that they need to do specific recovery logic when this fails yet, rather than panic.

I need to display a helpful error message to the user and indicate that this is an IO error, not a bug. Then the user will know if the error was because they spelled the database path wrong when giving it as a command line argument, or if they need to set file system permissions or make some free space. Even if the database is corrupt and can't be recovered, that's still an error on the user's side, and the user needs to be told that and not see a panic which means a bug in the program.

[spacejam]

Yep, that's why I panic with information that the open file fails. Feel free to submit a PR that improves the language of that error message.