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

feat(node): add Table.countRows() #185

Merged
merged 1 commit into from
Jun 15, 2023
Merged

Conversation

gsilvestrin
Copy link
Contributor

No description provided.

let (deferred, promise) = cx.promise();
let table = js_table.table.clone();

rt.block_on(async move {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be spawn instead of block_on? so that the promise can return before this task completes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spent some time trying to make spawm work here, but end up with the same "future is not send" error. I tried to refactor the mutex code and explicitly drop the mutex guard, but it did not help here.

error: future cannot be sent between threads safely
   --> rust/ffi/node/src/lib.rs:275:14
    |
275 |       rt.spawn(async move {
    |  ______________^
276 | |         let num_rows_result = table.lock().unwrap().count_rows().await;
277 | |
278 | |         deferred.settle_with(&channel, move |mut cx| {
...   |
281 | |         });
282 | |     });
    | |_____^ future created by async block is not `Send`
    |
    = help: within `[async block@rust/ffi/node/src/lib.rs:275:14: 282:6]`, the trait `std::marker::Send` is not implemented for `std::sync::MutexGuard<'_, Table>`
note: future is not `Send` as this value is used across an await

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. You can leave for a future PR then, but I think what we'll want to do is wrap the Mutex in an Arc.

fn table_count_rows(mut cx: FunctionContext) -> JsResult<JsPromise> {
let js_table = cx.this().downcast_or_throw::<JsBox<JsTable>, _>(&mut cx)?;
let rt = runtime(&mut cx)?;
let channel = cx.channel();
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use it in the deferred.settle_with to send the value back to the caller

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see 👍

@gsilvestrin gsilvestrin merged commit 78de8f5 into main Jun 15, 2023
7 checks passed
@gsilvestrin gsilvestrin deleted the gsilvestrin/nodejs_count_rows branch June 15, 2023 21:35
raghavdixit99 pushed a commit to raghavdixit99/lancedb that referenced this pull request Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants