neon-bindings / neon

Rust bindings for writing safe and fast native Node.js modules.
https://www.neon-bindings.com/
Apache License 2.0
8.01k stars 283 forks source link

Promise docs examples do not run on background thread #632

Closed That3Percent closed 3 years ago

That3Percent commented 3 years ago

The examples given at https://neon-bindings.com/docs/async/ don't actually run on the background thread as advertised.

For example:

// Iterate 10,0000 times in background thread
performAsyncTask((err, value) => {
  let count = 10;
  for (let i = 0; i < 100000; i++) {
    count++;
  }
  console.log(count, "first sum from background thread");
});

What is actually happening here is that a very tiny task is being run on the background thread: From BackgroundTask

fn perform(&self) -> Result<Self::Output, Self::Error> {
        Ok(17)
    }

perform takes very little time to execute, and after complete is called the callback is run on the main JavaScript thread.

The reason the output looks like this from the docs:

20 'second sum from main thread'
100010 'first sum from background thread'

is not because the JavaScript code is running in a separate thread, but rather because 'first sum from background thread' has been scheduled on the main thread to run after 'second sum`.

These examples re-enforce a couple of false assumptions, including that it is even possible to run JavaScript code threaded.

The Fibonacci example here gets it right, and may be a useful reference for the docs to draw from.

Edit: Re-worded slightly for clarity. The assumptions are false, but they are not dangerous.

kjvalencik commented 3 years ago

@That3Percent I think I might have experience bias because I don't see the part that is unclear. The page states that perform is the portion operated on a background thread.

Perform the task, producing either a successful Output or an unsuccessful Error. This method is executed in a background thread as part of libuv's built-in thread pool.

I agree that it could be clearer that the callback is not running on a separate thread. This example needs balance code complexity with expressing that the callback isn't executed immediately. The balance seems a little off.

Maybe if the summing part was removed and a little bit of extra work was added to perform? Fibonacci seems a bit too abstract for an example.

is not because the JavaScript code is running in a separate thread, but rather because 'first sum from background thread' has been scheduled on the main thread to run after 'second sum`.

The docs say the same thing, but avoid using the word word scheduled. Maybe if we specifically say, "if the callback were executed immediately"?

@amilajack I think you authored this example, do you have any thoughts on how to improve this?

That3Percent commented 3 years ago

@kjvalencik The examples make several explicit references to the JavaScript code being run on the background thread. I think it seems clear from a straightforward reading that the intent of the code was to run JavaScript in a way as to not block the main thread.

Here's an example: // Iterate 10,0000 times in background thread and "first sum from background thread"

Even if those comments were removed I think it to be misleading at best to have the bulk of the work not actually run in a background thread.

That3Percent commented 3 years ago

@kjvalencik To clarify, I have no problem with most of the docs, just the examples. If I were to nitpick the docs I'd object to the use of the word asynchronous when referring to callbacks but I think the necessary information can be pieced together from the docs that are there.

That3Percent commented 3 years ago

Maybe if the summing part was removed and a little bit of extra work was added to perform? Fibonacci seems a bit too abstract for an example.

Yes, I think that if the iteration code was moved to perform and just a console.log was in the callback this example would be much more clear.

kjvalencik commented 3 years ago

I skimmed over the code the first time and missed this. I see this incorrect comment repeated twice:

// Iterate 10,0000 times in background thread

But, I don't see the mistake repeated in the prose. Additionally, I don't see the value of the "this happens first" section. That appears to be a little too in the weeds of JavaScript scheduling without providing any benefit to the user.

@amilajack What do you think of moving the sum to perform and removing the part about synchronous code happening first?

@That3Percent what word would you use instead of asynchronous?

kjvalencik commented 3 years ago

We should also keep in mind that scheduling tasks on another thread will be changing significantly with the N-API backend. https://github.com/neon-bindings/rfcs/pull/32

We might not want to spend an enormous amount of time refactoring this page.

That3Percent commented 3 years ago

@That3Percent what word would you use instead of asynchronous?

After looking into this I think the word asynchronous might be less far off than I thought. But I'm not sure. Using this MDN page for reference terminology sometimes the word "asynchronously" is used almost a synonym for "non-blocking". But sometimes the words "async callback" are used, and "asynchronous callback" are used interchangeably so I feel this part fine.

amilajack commented 3 years ago

@kjvalencik yea I can take a look at this

kjvalencik commented 3 years ago

These docs were temporarily removed and will be rewritten.