fix(indexeddb): fix IDB cursor.continue_() call after drop

[borngraced]

If we try to get first result from indexeddb cursor like this

komodo-defi-framework/mm2src/coins/z_coin/storage/blockdb/blockdb_idb_storage.rs

Lines 90 to 98 in 272f46b

	let maybe_height = block_db
	.cursor_builder()
	.only("ticker", &ticker)?
	.bound("height", 0u32, u32::MAX)
	.reverse()
	.open_cursor(BlockDbTable::TICKER_HEIGHT_INDEX)
	.await?
	.next()
	.await?;

this error is being logged on the browser Uncaught Error: closure invoked recursively or after being dropped on the web because CursorDriver next loop is still looping due to this

komodo-defi-framework/mm2src/mm2_db/src/indexed_db/drivers/cursor/cursor.rs

Lines 286 to 302 in 272f46b

	match cursor_action {
	CursorAction::Continue => cursor.continue_().map_to_mm(|e| CursorError::AdvanceError {
	description: stringify_js_error(&e),
	})?,
	CursorAction::ContinueWithValue(next_value) => {
	cursor
	.continue_with_key(&next_value)
	.map_to_mm(|e| CursorError::AdvanceError {
	description: stringify_js_error(&e),
	})?
	},
	// Don't advance the cursor.
	// Here we set the `stopped` flag so we return `Ok(None)` at the next iteration immediately.
	// This is required because `item_action` can be `CollectItemAction::Include`,
	// and at this iteration we will return `Ok(Some)`.
	CursorAction::Stop => self.stopped = true,
	}

after getting the first value we requested and Cursor is dropped as we called next() just once here .

I found a fix which introduces a method called where_ that takes a closure. The closure returns a boolean, allowing you to control when a loop should stop. This approach works reliably in various situations.

Example - Simple Condition

When calling the next method once, you can easily obtain the first available result
without needing to continue the cursor using a simple closure like where_first():

let cursor_builder = CursorBuilder::new();

// Apply the default condition to the cursor builder to return the first item
let updated_cursor_builder = cursor_builder.where_first();

Example - Complex Condition

For more complex conditions, such as deserializing an IndexedDB data, handling errors, and checking a specific condition, you can use a closure like the one below:

let where_ = move |block| {
   serde_json::from_value::<BlockHeaderStorageTable>(block)
       .map_to_mm(|err| CursorError::ErrorDeserializingItem(err.to_string()))
       .map(|header| header.bits != max_bits)
};

// Apply the closure to the cursor builder using the where_ method
let updated_cursor_builder = cursor_builder.where_(where_);

related: #2019

[laruh]

@borngraced please fix wasm lint and PR name lint (like "allow first result using IDB cursor")

[borngraced]

@borngraced please fix wasm lint and PR name lint (like "allow first result using IDB cursor")

done

[onur-ozkan]

Great work!

At the end, it would be super nice to have unit tests for fn next and fn first in mm2src/mm2_db/src/indexed_db/drivers/cursor/cursor.rs module.

[onur-ozkan]

Instead of making NextItem complicated, what do you think adding a new event called FirstItem?

[onur-ozkan]

I believe this function should have consistent behavior without relying on conditional flags. Could we possibly create a new function called fn first and retain the existing function as is? The idea is to separate the common parts, which can then be called from both this function and fn first.

[borngraced]

thanks done this and it improved the code greatly!

[laruh]

yeah, agree, now it looks beautiful.

[laruh]

Will duplicate note in the PR.
fixes till this commit 7ae981b solve runtime error. However in the last commit closure invoked issues occur again.

Also there is a problem with oneshot canceled when mm2 was stopped

[artemii235]

However in the last commit closure invoked issues occur again.

I actually wanted to ask whether it solves the issue with cursor only when we need the first found item. I assume a more general fix is preferred because this one looks like a temporary workaround.

[laruh]

However in the last commit closure invoked issues occur again.

I actually wanted to ask whether it solves the issue with cursor only when we need the first found item. I assume a more general fix is preferred because this one looks like a temporary workaround.

It turned out that whole problem was in obtaining the first item. In CursorDriver::next method.
So fix should solve issue.

UPD: previously when we were trying to get one item by next(), the process continued in loop

[borngraced]

However in the last commit closure invoked issues occur again.

I actually wanted to ask whether it solves the issue with cursor only when we need the first found item. I assume a more general fix is preferred because this one looks like a temporary workaround.

CursorAction doesn't know Closure was dropped immediately we returned from the function that called
next() to get first result so it tries to send another event to get the next result just millisecs after CursorDriver/Closure was dropped so I think Implementing first() can be a good fix too as we also don't need to loop to request first result.
cc. @artemii235

pub(crate) async fn next(&mut self) -> CursorResult<Option<(ItemId, Json)>> {
        loop {
            ...
            match cursor_action {
                CursorAction::Continue => cursor.continue_().map_to_mm(|e| CursorError::AdvanceError {
                    description: stringify_js_error(&e),
                })?,
                CursorAction::ContinueWithValue(next_value) => {
                    cursor
                        .continue_with_key(&next_value)
                        .map_to_mm(|e| CursorError::AdvanceError {
                            description: stringify_js_error(&e),
                        })?
                },
                // Don't advance the cursor.
                // Here we set the `stopped` flag so we return `Ok(None)` at the next iteration immediately.
                // This is required because `item_action` can be `CollectItemAction::Include`,
                // and at this iteration we will return `Ok(Some)`.
                CursorAction::Stop => self.stopped = true,
            };

            match item_action {
                CursorItemAction::Include => return Ok(Some(item.into_pair())),
                // Try to fetch the next item.
                CursorItemAction::Skip => (),
            }
        }
    }


     /// if getting the first result is what we want
    pub(crate) async fn first(&mut self) -> CursorResult<Option<(ItemId, Json)>> {
        let event = match self.cursor_item_rx.next().await {
            Some(event) => event,
            None => return Ok(None),
        };

        let _cursor_event = event.map_to_mm(|e| CursorError::ErrorOpeningCursor {
            description: stringify_js_error(&e),
        })?;

        let cursor = match cursor_from_request(&self.cursor_request)? {
            Some(cursor) => cursor,
            // No more items.
            None => return Ok(None),
        };

        let (_, js_value) = match (cursor.key(), cursor.value()) {
            (Ok(key), Ok(js_value)) => (key, js_value),
            // No more items.
            _ => return Ok(None),
        };

        let item: InternalItem =
            deserialize_from_js(js_value).map_to_mm(|e| CursorError::ErrorDeserializingItem(e.to_string()))?;

        Ok(Some(item.into_pair()))
    }
    ```
    

[borngraced]

@laruh i improved the impl. so you can test it again. thanks

[laruh]

@borngraced recheck clippy or fmt please, CI fails

[borngraced]

@borngraced recheck clippy or fmt please, CI fails

not an issue with my pr? https://github.com/KomodoPlatform/komodo-defi-framework/actions/runs/7166781305/job/19511514652?pr=2028#step:3:8

[laruh]

not an issue with my pr? https://github.com/KomodoPlatform/komodo-defi-framework/actions/runs/7166781305/job/19511514652?pr=2028#step:3:8

Hmm lets rerun to check

UPD: yeah, its not your branch issue. locally its fine.

[borngraced]

I actually wanted to ask whether it solves the issue with cursor only when we need the first found item. I assume a more general fix is preferred because this one looks like a temporary workaround.

I agree with this, we should maybe have a limit function instead of first where we specify limit as 1 for first item. We can also add an until function which loops until a condition is found, maybe instead of until we can add where condition that can be used with limit. Since this case is not solved

komodo-defi-framework/mm2src/coins/utxo/utxo_block_header_storage/wasm/indexeddb_block_header_storage.rs

Lines 239 to 262 in 4b364de

	while let Some((_item_id, header)) = cursor
	.next()
	.await
	.map_err(|err| BlockHeaderStorageError::get_err(&ticker, err.to_string()))?
	{
	if header.bits == max_bits {
	continue;
	}
	let serialized = &hex::decode(header.raw_header).map_err(|e| BlockHeaderStorageError::DecodeError {
	coin: ticker.clone(),
	reason: e.to_string(),
	})?;
	let mut reader = Reader::new_with_coin_variant(serialized, ticker.as_str().into());
	let header: BlockHeader =
	reader
	.read()
	.map_err(|e: serialization::Error| BlockHeaderStorageError::DecodeError {
	coin: ticker,
	reason: e.to_string(),
	})?;
	return Ok(Some(header));
	}

After many hours of debugging I found another fix which introduces a method called where_ that takes a closure. The closure returns a boolean, allowing you to control when a loop should stop. This approach works reliably in various situations.

full explanation: #2028 (comment)

cc. @shamardy @laruh

[laruh]

@borngraced
can confirm that the last commit f8f1940 from impl_first_method_for_cursor branch + this commit from impl_first_method_for_cursor-test_nft_with_where_ branch dont have runtime errors

You can cherry pick the last commit from impl_first_method_for_cursor-test_nft_with_where_

[shamardy]

we should maybe have a limit function instead of first

@borngraced Thanks for the great find and fix. Can we use condition to request a certain number of items? I asked for a limit or similar functionalities to not get all items for cases like below where we currently fetch all items then we just return the limit requested (requested page).

komodo-defi-framework/mm2src/mm2_main/src/lp_swap/my_swaps_storage.rs

Lines 212 to 272 in e61c41e

	let items = match (&filter.my_coin, &filter.other_coin) {
	(Some(my_coin), Some(other_coin)) => {
	my_swaps_table
	.cursor_builder()
	.only("my_coin", my_coin)?
	.only("other_coin", other_coin)?
	.bound("started_at", from_timestamp, to_timestamp)
	.open_cursor("with_my_other_coins")
	.await?
	.collect()
	.await?
	},
	(Some(my_coin), None) => {
	my_swaps_table
	.cursor_builder()
	.only("my_coin", my_coin)?
	.bound("started_at", from_timestamp, to_timestamp)
	.open_cursor("with_my_coin")
	.await?
	.collect()
	.await?
	},
	(None, Some(other_coin)) => {
	my_swaps_table
	.cursor_builder()
	.only("other_coin", other_coin)?
	.bound("started_at", from_timestamp, to_timestamp)
	.open_cursor("with_other_coin")
	.await?
	.collect()
	.await?
	},
	(None, None) => {
	my_swaps_table
	.cursor_builder()
	.bound("started_at", from_timestamp, to_timestamp)
	.open_cursor("started_at")
	.await?
	.collect()
	.await?
	},
	};
	let uuids: BTreeSet<OrderedUuid> = items
	.into_iter()
	.map(|(_item_id, item)| OrderedUuid::from(item))
	.collect();
	match paging_options {
	Some(paging) => take_according_to_paging_opts(uuids, paging),
	None => {
	let total_count = uuids.len();
	Ok(MyRecentSwapsUuids {
	uuids_and_types: uuids
	.into_iter()
	.map(|ordered| (ordered.uuid, ordered.swap_type))
	.collect(),
	total_count,
	skipped: 0,
	})
	},
	}

You can see how limit and offset is used with sqlite to get only the values required

komodo-defi-framework/mm2src/mm2_main/src/database/my_swaps.rs

Lines 216 to 228 in e61c41e

	let skipped = match paging_options {
	Some(paging) => {
	// calculate offset, page_number is ignored if from_uuid is set
	let offset = match paging.from_uuid {
	Some(uuid) => offset_by_uuid(conn, &query_builder, &params, &uuid)?,
	None => (paging.page_number.get() - 1) * paging.limit,
	};
	query_builder.limit(paging.limit);
	query_builder.offset(offset);
	offset
	},
	None => 0,
	};

P.S. Maybe this enhancement can be done in another PR if it's too much to do here since it's unrelated to closure invoked recursively or after being dropped error. I actually don't know how much overhead we currently have by fetching all values.

[borngraced]

we should maybe have a limit function instead of first

@borngraced Thanks for the great find and fix. Can we use condition to request a certain number of items? I asked for a limit or similar functionalities to not get all items for cases like below where we currently fetch all items then we just return the limit requested (requested page).

komodo-defi-framework/mm2src/mm2_main/src/lp_swap/my_swaps_storage.rs

Lines 212 to 272 in e61c41e

	let items = match (&filter.my_coin, &filter.other_coin) {
	(Some(my_coin), Some(other_coin)) => {
	my_swaps_table
	.cursor_builder()
	.only("my_coin", my_coin)?
	.only("other_coin", other_coin)?
	.bound("started_at", from_timestamp, to_timestamp)
	.open_cursor("with_my_other_coins")
	.await?
	.collect()
	.await?
	},
	(Some(my_coin), None) => {
	my_swaps_table
	.cursor_builder()
	.only("my_coin", my_coin)?
	.bound("started_at", from_timestamp, to_timestamp)
	.open_cursor("with_my_coin")
	.await?
	.collect()
	.await?
	},
	(None, Some(other_coin)) => {
	my_swaps_table
	.cursor_builder()
	.only("other_coin", other_coin)?
	.bound("started_at", from_timestamp, to_timestamp)
	.open_cursor("with_other_coin")
	.await?
	.collect()
	.await?
	},
	(None, None) => {
	my_swaps_table
	.cursor_builder()
	.bound("started_at", from_timestamp, to_timestamp)
	.open_cursor("started_at")
	.await?
	.collect()
	.await?
	},
	};
	let uuids: BTreeSet<OrderedUuid> = items
	.into_iter()
	.map(|(_item_id, item)| OrderedUuid::from(item))
	.collect();
	match paging_options {
	Some(paging) => take_according_to_paging_opts(uuids, paging),
	None => {
	let total_count = uuids.len();
	Ok(MyRecentSwapsUuids {
	uuids_and_types: uuids
	.into_iter()
	.map(|ordered| (ordered.uuid, ordered.swap_type))
	.collect(),
	total_count,
	skipped: 0,
	})
	},
	}

You can see how limit and offset is used with sqlite to get only the values required

komodo-defi-framework/mm2src/mm2_main/src/database/my_swaps.rs

Lines 216 to 228 in e61c41e

	let skipped = match paging_options {
	Some(paging) => {
	// calculate offset, page_number is ignored if from_uuid is set
	let offset = match paging.from_uuid {
	Some(uuid) => offset_by_uuid(conn, &query_builder, &params, &uuid)?,
	None => (paging.page_number.get() - 1) * paging.limit,
	};
	query_builder.limit(paging.limit);
	query_builder.offset(offset);
	offset
	},
	None => 0,
	};

P.S. Maybe this enhancement can be done in another PR if it's too much to do here since it's unrelated to closure invoked recursively or after being dropped error. I actually don't know how much overhead we currently have by fetching all values.

We can use indexedb curosor.advance to achieve similar behavior as offset since there's no such functionalities in indexeded yet

and something like this for limit

const limit = 10;
let count = 0;
if cursor && count < limit {
  cursor.continue();
  count++;
}

but I'll research and explore other options for sure

[laruh]

I have small note :)

UPD not so small as I thought first time

[laruh]

Thanks! LGTM!

[shamardy]

Thank you for the fixes! A few more comments :)

[shamardy]

What would happen if there are more than one object/record that satisfies the condition? I see that we return the first object, are we sure we won't get Uncaught Error: closure invoked recursively or after being dropped. This is part of the reason I wanted where and limit, so we can get more than one object that satisfies the condition, if we want this in other cases (other than the case here where we want only the first object).

P.S. If we don't get the error when there are more than one object that satisfies the condition, then the inclusion of limit and refactoring of where to return more than one object if they exist can be done in a different PR since this PR purpose is to fix the error only.

[borngraced]

hmm this seems like another enhancement that's not related to this PR.

This PR only handle case for retrieving the first item that meets a condition(if specified) which solves js exception error: Uncaught Error: closure invoked recursively or after being But for sure this PR indeed fixed that and also optimizes calls to indexeddb.

I will work on the later issue during next print.

[shamardy]

LGTM but for a few nits! Please fix the first comment so that I can finally merge this PR.

[shamardy]

Re-approve