diff --git a/index-scheduler/src/batch.rs b/index-scheduler/src/batch.rs index 48eae0063..3e2cc4281 100644 --- a/index-scheduler/src/batch.rs +++ b/index-scheduler/src/batch.rs @@ -896,7 +896,7 @@ impl IndexScheduler { })?; // 4. Dump experimental feature settings - let features = self.features()?.runtime_features(); + let features = self.features().runtime_features(); dump.create_experimental_features(features)?; let dump_uid = started_at.format(format_description!( diff --git a/index-scheduler/src/error.rs b/index-scheduler/src/error.rs index d901129d2..ddc6960f7 100644 --- a/index-scheduler/src/error.rs +++ b/index-scheduler/src/error.rs @@ -113,8 +113,6 @@ pub enum Error { Dump(#[from] dump::Error), #[error(transparent)] Heed(#[from] heed::Error), - #[error("Unable to record metrics for request.")] - CannotRecordMetrics, #[error(transparent)] Milli(#[from] milli::Error), #[error("An unexpected crash occurred when processing the task.")] @@ -127,8 +125,6 @@ pub enum Error { Persist(#[from] tempfile::PersistError), #[error(transparent)] FeatureNotEnabled(#[from] FeatureNotEnabledError), - #[error("An unexpected error occurred when accessing the runtime features.")] - RuntimeFeatureToggleError, #[error(transparent)] Anyhow(#[from] anyhow::Error), @@ -181,14 +177,12 @@ impl Error { | Error::TaskCancelationWithEmptyQuery | Error::Dump(_) | Error::Heed(_) - | Error::CannotRecordMetrics | Error::Milli(_) | Error::ProcessBatchPanicked | Error::FileStore(_) | Error::IoError(_) | Error::Persist(_) | Error::FeatureNotEnabled(_) - | Error::RuntimeFeatureToggleError | Error::Anyhow(_) => true, Error::CreateBatch(_) | Error::CorruptedTaskQueue @@ -229,13 +223,11 @@ impl ErrorCode for Error { Error::Milli(e) => e.error_code(), Error::ProcessBatchPanicked => Code::Internal, Error::Heed(e) => e.error_code(), - Error::CannotRecordMetrics => Code::Internal, Error::HeedTransaction(e) => e.error_code(), Error::FileStore(e) => e.error_code(), Error::IoError(e) => e.error_code(), Error::Persist(e) => e.error_code(), Error::FeatureNotEnabled(_) => Code::FeatureNotEnabled, - Error::RuntimeFeatureToggleError => Code::Internal, // Irrecoverable errors Error::Anyhow(_) => Code::Internal, diff --git a/index-scheduler/src/features.rs b/index-scheduler/src/features.rs index 222e4a443..1db27bcd5 100644 --- a/index-scheduler/src/features.rs +++ b/index-scheduler/src/features.rs @@ -4,7 +4,6 @@ use meilisearch_types::features::{InstanceTogglableFeatures, RuntimeTogglableFea use meilisearch_types::heed::types::{SerdeJson, Str}; use meilisearch_types::heed::{Database, Env, RwTxn}; -use crate::error::Error::RuntimeFeatureToggleError; use crate::error::FeatureNotEnabledError; use crate::Result; @@ -22,9 +21,9 @@ pub struct RoFeatures { } impl RoFeatures { - fn new(data: &FeatureData) -> Result { - let runtime = data.runtime_features()?; - Ok(Self { runtime }) + fn new(data: &FeatureData) -> Self { + let runtime = data.runtime_features(); + Self { runtime } } pub fn runtime_features(&self) -> RuntimeTogglableFeatures { @@ -109,16 +108,22 @@ impl FeatureData { self.persisted.put(&mut wtxn, EXPERIMENTAL_FEATURES, &features)?; wtxn.commit()?; - let mut toggled_features = self.runtime.write().map_err(|_| RuntimeFeatureToggleError)?; + // safe to unwrap, the lock will only fail if: + // 1. requested by the same thread concurrently -> it is called and released in methods that don't call each other + // 2. there's a panic while the thread is held -> it is only used for an assignment here. + let mut toggled_features = self.runtime.write().unwrap(); *toggled_features = features; Ok(()) } - fn runtime_features(&self) -> Result { - Ok(*self.runtime.read().map_err(|_| RuntimeFeatureToggleError)?) + fn runtime_features(&self) -> RuntimeTogglableFeatures { + // sound to unwrap, the lock will only fail if: + // 1. requested by the same thread concurrently -> it is called and released in methods that don't call each other + // 2. there's a panic while the thread is held -> it is only used for copying the data here + *self.runtime.read().unwrap() } - pub fn features(&self) -> Result { + pub fn features(&self) -> RoFeatures { RoFeatures::new(self) } } diff --git a/index-scheduler/src/lib.rs b/index-scheduler/src/lib.rs index b2a263355..43ac2355c 100644 --- a/index-scheduler/src/lib.rs +++ b/index-scheduler/src/lib.rs @@ -579,13 +579,7 @@ impl IndexScheduler { run.wake_up.wait(); loop { - let puffin_enabled = match run.features() { - Ok(features) => features.check_puffin().is_ok(), - Err(e) => { - log::error!("{e}"); - continue; - } - }; + let puffin_enabled = run.features().check_puffin().is_ok(); puffin::set_scopes_on(puffin_enabled); puffin::GlobalProfiler::lock().new_frame(); @@ -1299,7 +1293,7 @@ impl IndexScheduler { Ok(IndexStats { is_indexing, inner_stats: index_stats }) } - pub fn features(&self) -> Result { + pub fn features(&self) -> RoFeatures { self.features.features() } diff --git a/meilisearch/src/middleware.rs b/meilisearch/src/middleware.rs index 7f7429186..5b87dee34 100644 --- a/meilisearch/src/middleware.rs +++ b/meilisearch/src/middleware.rs @@ -6,9 +6,7 @@ use actix_web::dev::{self, Service, ServiceRequest, ServiceResponse, Transform}; use actix_web::web::Data; use actix_web::Error; use futures_util::future::LocalBoxFuture; -use index_scheduler::Error::CannotRecordMetrics; use index_scheduler::IndexScheduler; -use meilisearch_types::error::ResponseError; use prometheus::HistogramTimer; pub struct RouteMetrics; @@ -55,14 +53,7 @@ where // calling unwrap here is safe because index scheduler is added to app data while creating actix app. // also, the tests will fail if this is not present. let index_scheduler = req.app_data::>().unwrap(); - let features = match index_scheduler.features() { - Ok(features) => features, - Err(_e) => { - return Box::pin( - async move { Err(ResponseError::from(CannotRecordMetrics).into()) }, - ); - } - }; + let features = index_scheduler.features(); if features.check_metrics().is_ok() { let request_path = req.path(); diff --git a/meilisearch/src/routes/features.rs b/meilisearch/src/routes/features.rs index 4437f602d..e7fd8de22 100644 --- a/meilisearch/src/routes/features.rs +++ b/meilisearch/src/routes/features.rs @@ -29,12 +29,12 @@ async fn get_features( >, req: HttpRequest, analytics: Data, -) -> Result { - let features = index_scheduler.features()?; +) -> HttpResponse { + let features = index_scheduler.features(); analytics.publish("Experimental features Seen".to_string(), json!(null), Some(&req)); debug!("returns: {:?}", features.runtime_features()); - Ok(HttpResponse::Ok().json(features.runtime_features())) + HttpResponse::Ok().json(features.runtime_features()) } #[derive(Debug, Deserr)] @@ -59,7 +59,7 @@ async fn patch_features( req: HttpRequest, analytics: Data, ) -> Result { - let features = index_scheduler.features()?; + let features = index_scheduler.features(); let old_features = features.runtime_features(); let new_features = meilisearch_types::features::RuntimeTogglableFeatures { diff --git a/meilisearch/src/routes/indexes/facet_search.rs b/meilisearch/src/routes/indexes/facet_search.rs index 831c45a85..142a424c0 100644 --- a/meilisearch/src/routes/indexes/facet_search.rs +++ b/meilisearch/src/routes/indexes/facet_search.rs @@ -68,7 +68,7 @@ pub async fn search( } let index = index_scheduler.index(&index_uid)?; - let features = index_scheduler.features()?; + let features = index_scheduler.features(); let search_result = tokio::task::spawn_blocking(move || { perform_facet_search(&index, search_query, facet_query, facet_name, features) }) diff --git a/meilisearch/src/routes/indexes/search.rs b/meilisearch/src/routes/indexes/search.rs index 3eabf61f3..5a0a9e92b 100644 --- a/meilisearch/src/routes/indexes/search.rs +++ b/meilisearch/src/routes/indexes/search.rs @@ -157,7 +157,7 @@ pub async fn search_with_url_query( let mut aggregate = SearchAggregator::from_query(&query, &req); let index = index_scheduler.index(&index_uid)?; - let features = index_scheduler.features()?; + let features = index_scheduler.features(); let search_result = tokio::task::spawn_blocking(move || perform_search(&index, query, features)).await?; if let Ok(ref search_result) = search_result { @@ -192,7 +192,7 @@ pub async fn search_with_post( let index = index_scheduler.index(&index_uid)?; - let features = index_scheduler.features()?; + let features = index_scheduler.features(); let search_result = tokio::task::spawn_blocking(move || perform_search(&index, query, features)).await?; if let Ok(ref search_result) = search_result { diff --git a/meilisearch/src/routes/metrics.rs b/meilisearch/src/routes/metrics.rs index af1f2b536..7a13a758f 100644 --- a/meilisearch/src/routes/metrics.rs +++ b/meilisearch/src/routes/metrics.rs @@ -19,7 +19,7 @@ pub async fn get_metrics( index_scheduler: GuardedData, Data>, auth_controller: Data, ) -> Result { - index_scheduler.features()?.check_metrics()?; + index_scheduler.features().check_metrics()?; let auth_filters = index_scheduler.filters(); if !auth_filters.all_indexes_authorized() { let mut error = ResponseError::from(AuthenticationError::InvalidToken); diff --git a/meilisearch/src/routes/multi_search.rs b/meilisearch/src/routes/multi_search.rs index e257af1cf..3a028022a 100644 --- a/meilisearch/src/routes/multi_search.rs +++ b/meilisearch/src/routes/multi_search.rs @@ -41,7 +41,7 @@ pub async fn multi_search_with_post( let queries = params.into_inner().queries; let mut multi_aggregate = MultiSearchAggregator::from_queries(&queries, &req); - let features = index_scheduler.features()?; + let features = index_scheduler.features(); // Explicitly expect a `(ResponseError, usize)` for the error type rather than `ResponseError` only, // so that `?` doesn't work if it doesn't use `with_index`, ensuring that it is not forgotten in case of code