fix: RwLock race condition during group controller instantiation (App…

…Flowy-IO#6860)

* chore: code cleanup

* fix: view editor used during initialization

* fix: quick and dirty hack job

* test: add test

* chore: don't create separate mut var

* chore: docs

* fix: uninitialized group controllers

* chore: remove group

* chore: fix test

---------

Co-authored-by: nathan <nathan@appflowy.io>

frontend/appflowy_flutter/lib/plugins/database/application/tab_bar_bloc.dart

@@ -293,7 +293,6 @@ class DatabaseTabBarController {
   OnViewChildViewChanged? onViewChildViewChanged;
 
   Future<void> dispose() async {
-    await viewListener.stop();
-    await controller.dispose();
+    await Future.wait([viewListener.stop(), controller.dispose()]);
   }
 }

frontend/appflowy_flutter/lib/plugins/database/board/presentation/board_page.dart

@@ -179,17 +179,15 @@ class _DesktopBoardPageState extends State<DesktopBoardPage> {
     _focusScope.dispose();
     _boardBloc.close();
     _boardActionsCubit.close();
-    _didCreateRow
-      ..removeListener(_handleDidCreateRow)
-      ..dispose();
+    _didCreateRow.dispose();
     super.dispose();
   }
 
   @override
   Widget build(BuildContext context) {
     return MultiBlocProvider(
       providers: [
-        BlocProvider<BoardBloc>.value(
+        BlocProvider.value(
           value: _boardBloc,
         ),
         BlocProvider.value(

frontend/rust-lib/flowy-database2/src/services/database_view/view_editor.rs

@@ -70,6 +70,14 @@ impl Drop for DatabaseViewEditor {
 }
 
 impl DatabaseViewEditor {
+  /// Create a new Database View Editor.
+  ///
+  /// After creating the editor, you must call [DatabaseViewEditor::initialize] to properly initialize it.
+  /// This initialization step will load essential data, such as group information.
+  ///
+  /// Avoid calling any methods of [DatabaseViewOperation] before the editor is fully initialized,
+  /// as some actions may rely on the current editor state. Failing to follow this order could result
+  /// in unexpected behavior, including potential deadlocks.
   pub async fn new(
     database_id: String,
     view_id: String,
@@ -127,6 +135,16 @@ impl DatabaseViewEditor {
     })
   }
 
+  /// Initialize the editor after creating it
+  /// You should call [DatabaseViewEditor::initialize] after creating the editor
+  pub async fn initialize(&self) -> FlowyResult<()> {
+    if let Some(group) = self.group_controller.write().await.as_mut() {
+      group.load_group_data().await?;
+    }
+
+    Ok(())
+  }
+
   pub async fn insert_row(&self, row: Option<Arc<Row>>, index: u32, row_order: &RowOrder) {
     let mut row_orders = self.row_orders.write().await;
     if row_orders.len() >= index as usize {
@@ -975,15 +993,17 @@ impl DatabaseViewEditor {
     if let Some(field) = self.delegate.get_field(field_id).await {
       tracing::trace!("create new group controller");
 
-      let new_group_controller = new_group_controller(
+      let mut new_group_controller = new_group_controller(
         self.view_id.clone(),
         self.delegate.clone(),
         self.filter_controller.clone(),
         Some(field),
       )
       .await?;
 
-      if let Some(controller) = &new_group_controller {
+      if let Some(controller) = &mut new_group_controller {
+        (*controller).load_group_data().await?;
+
         let new_groups = controller
           .get_all_groups()
           .into_iter()
@@ -1113,13 +1133,22 @@ impl DatabaseViewEditor {
     }
 
     // initialize the group controller if the current layout support grouping
-    *self.group_controller.write().await = new_group_controller(
+    let new_group_controller = match new_group_controller(
       self.view_id.clone(),
       self.delegate.clone(),
       self.filter_controller.clone(),
       None,
     )
-    .await?;
+    .await?
+    {
+      Some(mut controller) => {
+        controller.load_group_data().await?;
+        Some(controller)
+      },
+      None => None,
+    };
+
+    *self.group_controller.write().await = new_group_controller;
 
     let payload = DatabaseLayoutMetaPB {
       view_id: self.view_id.clone(),

frontend/rust-lib/flowy-database2/src/services/database_view/view_group.rs

@@ -27,14 +27,13 @@ pub async fn new_group_controller(
 
   let controller_delegate = GroupControllerDelegateImpl {
     delegate: delegate.clone(),
-    filter_controller: filter_controller.clone(),
+    filter_controller,
   };
 
   let grouping_field = match grouping_field {
     Some(field) => Some(field),
     None => {
       let group_setting = controller_delegate.get_group_setting(&view_id).await;
-
       let fields = delegate.get_fields(&view_id, None).await;
 
       group_setting

frontend/rust-lib/flowy-database2/src/services/database_view/views.rs

@@ -61,8 +61,10 @@ impl DatabaseViews {
       return Ok(editor.clone());
     }
 
-    let mut editor_map = self.view_editors.write().await;
     let database_id = self.database.read().await.get_database_id();
+    // Acquire the write lock to insert the new editor. We need to acquire the lock first to avoid
+    // initializing the editor multiple times.
+    let mut editor_map = self.view_editors.write().await;
     let editor = Arc::new(
       DatabaseViewEditor::new(
         database_id,
@@ -73,6 +75,9 @@ impl DatabaseViews {
       .await?,
     );
     editor_map.insert(view_id.to_owned(), editor.clone());
+    drop(editor_map);
+
+    editor.initialize().await?;
     Ok(editor)
   }
 

frontend/rust-lib/flowy-database2/src/services/group/action.rs

@@ -98,6 +98,7 @@ pub trait GroupCustomize: Send + Sync {
 /// while a `URL` group controller will be a `DefaultGroupController`.
 #[async_trait]
 pub trait GroupController: Send + Sync {
+  async fn load_group_data(&mut self) -> FlowyResult<()>;
   /// Returns the id of field that is being used to group the rows
   fn get_grouping_field_id(&self) -> &str;
 

frontend/rust-lib/flowy-database2/src/services/group/controller.rs

@@ -54,20 +54,12 @@ where
 {
   pub async fn new(
     grouping_field: &Field,
-    mut configuration: GroupControllerContext<C>,
+    context: GroupControllerContext<C>,
     delegate: Arc<dyn GroupControllerDelegate>,
   ) -> FlowyResult<Self> {
-    let field_type = FieldType::from(grouping_field.field_type);
-    let type_option = grouping_field
-      .get_type_option::<T>(&field_type)
-      .unwrap_or_else(|| T::from(default_type_option_data_from_type(field_type)));
-
-    let generated_groups = G::build(grouping_field, &configuration, &type_option).await;
-    let _ = configuration.init_groups(generated_groups)?;
-
     Ok(Self {
       grouping_field_id: grouping_field.id.clone(),
-      context: configuration,
+      context,
       group_builder_phantom: PhantomData,
       cell_parser_phantom: PhantomData,
       delegate,
@@ -163,6 +155,30 @@ where
   G: GroupsBuilder<Context = GroupControllerContext<C>, GroupTypeOption = T>,
   Self: GroupCustomize<GroupTypeOption = T>,
 {
+  async fn load_group_data(&mut self) -> FlowyResult<()> {
+    let grouping_field = self
+      .delegate
+      .get_field(&self.grouping_field_id)
+      .await
+      .ok_or_else(|| FlowyError::internal().with_context("Failed to get grouping field"))?;
+
+    let field_type = FieldType::from(grouping_field.field_type);
+    let type_option = grouping_field
+      .get_type_option::<T>(&field_type)
+      .unwrap_or_else(|| T::from(default_type_option_data_from_type(field_type)));
+
+    let generated_groups = G::build(&grouping_field, &self.context, &type_option).await;
+    let _ = self.context.init_groups(generated_groups)?;
+
+    let row_details = self.delegate.get_all_rows(&self.context.view_id).await;
+    let rows = row_details
+      .iter()
+      .map(|row| row.as_ref())
+      .collect::<Vec<_>>();
+    self.fill_groups(rows.as_slice(), &grouping_field)?;
+    Ok(())
+  }
+
   fn get_grouping_field_id(&self) -> &str {
     &self.grouping_field_id
   }

frontend/rust-lib/flowy-database2/src/services/group/controller_impls/date_controller.rs

@@ -219,7 +219,7 @@ impl GroupCustomize for DateGroupController {
   fn will_create_row(&self, cells: &mut Cells, field: &Field, group_id: &str) {
     match self.context.get_group(group_id) {
       None => tracing::warn!("Can not find the group: {}", group_id),
-      Some((_, _)) => {
+      _ => {
         let date = DateTime::parse_from_str(group_id, GROUP_ID_DATE_FORMAT).unwrap();
         let cell = insert_date_cell(date.timestamp(), None, Some(false), field);
         cells.insert(field.id.clone(), cell);

frontend/rust-lib/flowy-database2/src/services/group/controller_impls/default_controller.rs

@@ -21,6 +21,7 @@ use crate::services::group::{
 /// means all rows will be grouped in the same group.
 ///
 pub struct DefaultGroupController {
+  pub view_id: String,
   pub field_id: String,
   pub group: GroupData,
   pub delegate: Arc<dyn GroupControllerDelegate>,
@@ -29,9 +30,10 @@ pub struct DefaultGroupController {
 const DEFAULT_GROUP_CONTROLLER: &str = "DefaultGroupController";
 
 impl DefaultGroupController {
-  pub fn new(field: &Field, delegate: Arc<dyn GroupControllerDelegate>) -> Self {
+  pub fn new(view_id: &str, field: &Field, delegate: Arc<dyn GroupControllerDelegate>) -> Self {
     let group = GroupData::new(DEFAULT_GROUP_CONTROLLER.to_owned(), field.id.clone(), true);
     Self {
+      view_id: view_id.to_owned(),
       field_id: field.id.clone(),
       group,
       delegate,
@@ -41,6 +43,19 @@ impl DefaultGroupController {
 
 #[async_trait]
 impl GroupController for DefaultGroupController {
+  async fn load_group_data(&mut self) -> FlowyResult<()> {
+    let row_details = self.delegate.get_all_rows(&self.view_id).await;
+    let rows = row_details
+      .iter()
+      .map(|row| row.as_ref())
+      .collect::<Vec<_>>();
+
+    rows.iter().for_each(|row| {
+      self.group.add_row((*row).clone());
+    });
+    Ok(())
+  }
+
   fn get_grouping_field_id(&self) -> &str {
     &self.field_id
   }

frontend/rust-lib/flowy-database2/src/services/group/controller_impls/select_option_controller/multi_select_controller.rs

@@ -114,6 +114,7 @@ impl GroupCustomize for MultiSelectGroupController {
     {
       // Remove the option if the group is found
       new_type_option.options.remove(option_index);
+      self.context.delete_group(group_id)?;
       Ok(Some(new_type_option.into()))
     } else {
       Ok(None)

frontend/rust-lib/flowy-database2/src/services/group/controller_impls/select_option_controller/single_select_controller.rs

@@ -116,6 +116,7 @@ impl GroupCustomize for SingleSelectGroupController {
     {
       // Remove the option if the group is found
       new_type_option.options.remove(option_index);
+      self.context.delete_group(group_id)?;
       Ok(Some(new_type_option.into()))
     } else {
       Ok(None)

frontend/rust-lib/flowy-database2/src/services/group/group_builder.rs

@@ -63,7 +63,7 @@ pub type UpdatedCells = HashMap<String, Cell>;
   fields(grouping_field_id=%grouping_field.id, grouping_field_type)
   err
 )]
-pub async fn make_group_controller<D>(
+pub(crate) async fn make_group_controller<D>(
   view_id: &str,
   grouping_field: Field,
   delegate: D,
@@ -74,7 +74,7 @@ where
   let grouping_field_type = FieldType::from(grouping_field.field_type);
   tracing::Span::current().record("grouping_field", &grouping_field_type.default_name());
 
-  let mut group_controller: Box<dyn GroupController>;
+  let group_controller: Box<dyn GroupController>;
   let delegate = Arc::new(delegate);
 
   match grouping_field_type {
@@ -135,20 +135,13 @@ where
     },
     _ => {
       group_controller = Box::new(DefaultGroupController::new(
+        view_id,
         &grouping_field,
         delegate.clone(),
       ));
     },
   }
 
-  // Separates the rows into different groups
-  let row_details = delegate.get_all_rows(view_id).await;
-  let rows = row_details
-    .iter()
-    .map(|row| row.as_ref())
-    .collect::<Vec<_>>();
-
-  group_controller.fill_groups(rows.as_slice(), &grouping_field)?;
   #[cfg(feature = "verbose_log")]
   {
     for group in group_controller.get_all_groups() {

frontend/rust-lib/flowy-database2/tests/database/group_test/url_group_test.rs

@@ -78,3 +78,25 @@ async fn group_move_url_group_row_test() {
   test.assert_group_row_count(1, 3).await;
   test.assert_group_row_count(2, 1).await;
 }
+
+// Create a URL field in the default board and then set it as the grouping field.
+#[tokio::test]
+async fn set_group_by_url_field_test() {
+  let test = DatabaseGroupTest::new().await;
+  let url_field = test.get_url_field().await;
+
+  // group by URL field
+  test
+    .editor
+    .set_group_by_field(&test.view_id, &url_field.id, vec![])
+    .await
+    .unwrap();
+
+  // assert number of groups
+  test.assert_group_count(3).await;
+
+  // close the database view
+  test.editor.close_view(&test.view_id).await;
+
+  test.assert_group_count(3).await;
+}