Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the new safe read-txn-no-tls heed feature #3952

Merged
merged 1 commit into from Jul 26, 2023

Conversation

Kerollmops
Copy link
Member

@Kerollmops Kerollmops commented Jul 26, 2023

We recently found out that the read-sync-txn heed feature was invalid and must be removed from this crate. We were declaring it in milli/meilisearch but, fortunately, not sharing the RoTxns across threads 😮‍💨

I recently introduced the read-txn-no-tls heed feature, which implements RoTxn: Send and allows multiple read transactions on a single thread (which we use).

This PR replaces the sync-read-txn heed feature with the read-txn-no-tls one in the Cargo.toml file. I will fix this in heed v0.20.0 and will fill a RustSec advisory in the meantime.

@Kerollmops Kerollmops added the security Address a security vulnerability label Jul 26, 2023
@Kerollmops Kerollmops added this to the v1.3.0 milestone Jul 26, 2023
curquiza
curquiza previously approved these changes Jul 26, 2023
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked with @Kerollmops, let's merge it!

bors merge

meili-bors bot added a commit that referenced this pull request Jul 26, 2023
3952: No more use the broken `sync-read-txn` heed feature r=curquiza a=Kerollmops

[We recently found out](meilisearch/heed#191 (comment)) that the `read-sync-txn` heed feature was invalid and must be removed from this crate. We were declaring it in milli/meilisearch but, fortunately, not sharing the `RoTxn`s across threads 😮‍💨

This PR removes the `sync-read-txn` heed feature form the _Cargo.toml_ file. I will fix this in heed v0.20.0 and will fill a RustSec advisory in the meantime.

Co-authored-by: Clément Renault <clement@meilisearch.com>
@meili-bors
Copy link
Contributor

meili-bors bot commented Jul 26, 2023

Build failed:

@Kerollmops
Copy link
Member Author

Kerollmops commented Jul 26, 2023

It is failing due to the removal of the sync-read-txn feature (obviously), which wasn't only implementing RoTxn: Sync but also making it possible to have multiple RoTxn on the same thread (the real reason why tests fails). This is why I renamed the flag and I will be releasing a new version of heed v0.12.7, which introduces a new valid and safe read-txn-no-tls feature implementing RoTxn: Send and making it possible to have multiple RoTxn on the same thread.

@Kerollmops Kerollmops changed the title No more use the broken sync-read-txn heed feature Use the new safe read-txn-no-tls heed feature Jul 26, 2023
Copy link
Member

@ManyTheFish ManyTheFish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented Jul 26, 2023

Build succeeded:

@Kerollmops Kerollmops merged commit 3a34142 into release-v1.3.0 Jul 26, 2023
10 checks passed
@Kerollmops Kerollmops deleted the remove-heed-security branch July 26, 2023 17:18
@meili-bot meili-bot added the v1.3.0 PRs/issues solved in v1.3.0 released on 2023-07-31 label Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Address a security vulnerability v1.3.0 PRs/issues solved in v1.3.0 released on 2023-07-31
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants