Refactor error type to use thiserror

[ShadowJonathan]

Currently, when trying to convert or use heed for ?-error purposes, i get the following;

error[E0277]: `(dyn std::error::Error + 'static)` cannot be shared between threads safely
  --> tools\migrate\src\main.rs:17:68
   |
17 |             "heed" => Self::Heed(HeedDB::new(db::heed::new_db(path)?)),
   |                                                                    ^ `(dyn std::error::Error + 'static)` cannot be shared between threads safely
   |
   = help: the trait `Sync` is not implemented for `(dyn std::error::Error + 'static)`
   = note: required because of the requirements on the impl of `Sync` for `Unique<(dyn std::error::Error + 'static)>`
   = note: required because it appears within the type `Box<(dyn std::error::Error + 'static)>`
   = note: required because it appears within the type `heed::Error`
   = note: required because of the requirements on the impl of `From<heed::Error>` for `anyhow::Error`
   = note: required by `from`

The error type is a composite enum, which is fine, but it argues it cannot be sent between threads safely because the trait isn't marked, or valid.

I could also ask to wrangle the autotrait in, but i think the error type will also benefit from a bit more readability and maintainance if it's implemented with thiserror, for example like here

[Kerollmops]

Hey @ShadowJonathan,

Which version of heed are you using? I wrote the version 0.11 to try using real error types when casting bytes by using the bytemuck library instead of the zerocopy one. This had great benefits as the BytesDe/Encode were able to return Results with a real error type instead of Options helping debug a lot.

However, I miss something important: The Error::En/Decoding variants do not force the Boxed De/Encoding error to be Send + Sync + staticonly the fact that it implementsstd::error::Error`. This is a mistake and knows I am not sure about the 0.11 version itself, this is why I roll back to the 0.10.6 and moved from there with the 0.12, this 0.12 version uses an up-to-date version of LMDB that is a fork of the lmdb-sys repository of Mozilla.

Using the latest version of LMDB makes it work correctly on windows. I also applied a "patch" to the LMDB version we are using, I introduced a new MDB_ALWAYSFREEPAGES flag that limits the amount of memory used when writing big updates in LMDB.

If you want to enjoy all of this stuff you can use the tag v0.12.1 or newer. If you want to continue using the 0.11 version with that returns real parsing errors maybe we can release a v0.13 version with the patch I described above (Send + Sync + 'static).

[ShadowJonathan]

I'm using @timokoesters's fork, which is based on v0.11v0.10.6, thanks for mentioning the newer version though, I'll poke him to update the fork to that, because we're having problems making it work nicely on windows.

Edit: going ahead with the 'static patch seems a right thing to do, I think that's a good choice.

[Kerollmops]

I think that the PR that @timokoesters has done is to be merged on the new v0.12 that isn't published on crates.io for now.

Edit: going ahead with the 'static patch seems a right thing to do, I think that's a good choice.

If you want you could also stop using the new 0.11 and use v0.12.1 version instead, only this version has the LMDB latest version that works better for Windows.

[ShadowJonathan]

Two questions;

• Do you have an IM handle (discord, twitter, matrix, etc.) where i can contact you on? There's some questions about heed's development which i might want to ask.

• When is that version going to be merged into master? Looking at the tag, it's on the v0.12 branch, which is some commits behind master.

[Kerollmops]

Hey @ShadowJonathan,

Do you have an IM handle (discord, twitter, matrix, etc.) where i can contact you on? There's some questions about heed's development which i might want to ask.

I just sent you an email with my Discord handle 😃

When is that version going to be merged into master? Looking at the tag, it's on the v0.12 branch, which is some commits behind master.

In fact, the v0.12 is not real "behind" master, I tried some funny things on the v0.11 version (master) and I wasn't quite satisfied by the result (I want GAT to be out in stable Rust 😫). Then we discovered that the LMDB version that is binded by Mozilla in lmdb-rkv-sys was quite old and therefore didn't include most fixes to make LMDB work correctly on Windows, so, at MeiliSearch, we decided to fork this crate and to bump the bonded version to the latest, I patched the LMDB library itself to always free pages which helps to reduce the amount of memory used when doing a lot of modification.

We are currently waiting for some internal subject to be cleared to publish this lmdb-rkv-sys fork to crates.io, when done we will be able to publish a new version of heed (v0.12) that uses this update-to-date and "patched" LMDB binding crate.

[ShadowJonathan]

Alright, I still got some questions about master, but I'll ask them over IM

Back to the topic at hand; I think I'll probably make a PR which closes this, and makes error handling a bit less tedious (and more ergonomic rust) with thiserror, the "correct" error trait bounds would be a side effect then, but I just think using thiserror could be a boon.