Reactive-Extensions / RxJS

The Reactive Extensions for JavaScript
http://reactivex.io
Other
19.48k stars 2.1k forks source link

While loop blocks the thread #474

Closed Fresheyeball closed 9 years ago

Fresheyeball commented 9 years ago

JavaScript is single threaded, so wont this while loop block the thread? What does concurrency mean in this case? I'm seeing references to threads in the JavaScript, what does that mean?

https://github.com/Reactive-Extensions/RxJS/blob/master/src/core/concurrency/immediatescheduler.js#L8

    function scheduleRelative(state, dueTime, action) {
      var dt = normalizeTime(dueTime);
      while (dt - this.now() > 0) { }
      return action(this, state);
    }
paulpdaniels commented 9 years ago

You are correct, immediate scheduler will block until all tasks are complete. There is no concurrent behavior in this case.

Fresheyeball commented 9 years ago

What do you mean by

block until all tasks are complete

There is only one thread, so a blocked thread means no work is being accomplished. How can tasks become complete if the thread is blocked?

paulpdaniels commented 9 years ago

Sorry I wasn't clear on that, the while loop is preventing forward progress until now() reaches the scheduled time of the action, at which point it executes the action and returns. If you had a bunch of actions you wanted to schedule say at 1, 2 and 3 seconds. The result of scheduling them would be as follows:


scheduler.scheduleRelative(0, 1000, someAction); //Block and do no other work until 1 second has passed

//~1000 milliseconds later some action has just been completed
//2 seconds
scheduler.scheduleRelative(0, 1000, someOtherAction);

//1000 milliseconds later
//3 seconds
scheduler.scheduleRelatvie(0, 1000, someFinalAction);

This behavior is about what you would expect, however whereas a normal timeout scheduler might use setTimeout to schedule the action some time down the road and tell the javascript engine to go on doing other things in the meantime, the immediate scheduler will just busy wait until the clock catches up.

Does that clear it up?

Fresheyeball commented 9 years ago

Yes, and we should never deliberately introduce a busy wait in a single threaded ui system. If this is intended as an immediate scheduler, why not just execute right away? Or why not just use Scheduler.currentThread and set its dueTime to now?

I feel like I'm missing something here.

Fresheyeball commented 9 years ago
scheduler.scheduleRelative(0, 1000, someAction); //Block and do no other work until 1 second has passed

//~1000 milliseconds later some action has just been completed
//2 seconds
scheduler.scheduleRelative(0, 1000, someOtherAction);

//1000 milliseconds later
//3 seconds
scheduler.scheduleRelatvie(0, 1000, someFinalAction);

This too could be accomplished without the busy wait.

mattpodwysocki commented 9 years ago

@Fresheyeball @paulpdaniels I'm not seeing an issue here other than you shouldn't use the immediate scheduler nor current thread for any future based operation in JavaScript. In other runtime environments where thread blocking is ok and doesn't hang the browser, that's fine.

What is the desired outcome here?

Fresheyeball commented 9 years ago

First I want to understand. Second, if I still feel its a problem, I want to help submitting or collaborating on a solution.

Right now this looks like a JS lib written by people who are used to threads, and that can cause some problems. So understanding why things like busy waits are deliberately in the code are really important.

Fresheyeball commented 9 years ago

http://www.phpied.com/css-animations-off-the-ui-thread/ In this example the "kill switch" is a busy wait.

function kill() {
  var start = +new Date;
  while (+new Date - start < 2000){}
}

@mattpodwysocki what is the "runtime environment" you speak of where the animation will not be blocked by the busy wait?

Fresheyeball commented 9 years ago

I understand the "immediate scheduler" should not be used for future operations. But a developer can misuse this or have a bug regarding .now() that results in thread block. This is dangerous, especially considering that if the intent is the synchronously run right away, the busy wait should never happen, so it only introduces risk without benefit.

mattpodwysocki commented 9 years ago

@Fresheyeball I'm not talking about animations here, because Node.js doesn't complain about a blocking thread, nor does WSH or any other server based environment. The question I have to ask is why are you using immediate or current thread for future scheduling when obviously you need Rx.Scheduler.timeout?

mattpodwysocki commented 9 years ago

@Fresheyeball to that point, we've clearly documented which Schedulers should be used for what: https://github.com/Reactive-Extensions/RxJS/blob/master/doc/gettingstarted/schedulers.md

Fresheyeball commented 9 years ago

Node.js is definitely effected by blocking threads for the same reasons as animation (this should be obvious as both google chrome and node.js are using the same javascript runtime), and I'm not intent on deliberately misusing the immediate scheduler, but I am bothered by the risk this introduces for no clear benefit.

mattpodwysocki commented 9 years ago

@Fresheyeball there are the same risks all over the place, for example, calling distinct on a very large collection, calling reduce on a collection with no end. There may be some cases where you do want to block the world until the processing is complete, so I'm not willing to remove it. Discourage it, sure.

Fresheyeball commented 9 years ago

There are no infinite collections in JavaScript, so reduce will always terminate. Also reduce and distinct have objective benefits, that make risk tolerable (again this is a value judgement, the value of reduce is greater than the risk of it being misused).

In this thread, which is entirely about questioning the benefit of the busy wait, no one has stated the value of the busy wait. There are cases where you want to block until processing is complete, and in single threaded code, you block with that processing (all processing is blocking). A busy wait would prevent processing from completing, so if the goal of the busy wait is to block until processing is complete, it does not do that, because we only have one thread. A busy wait can ONLY block for no benefit in single threaded code.

Furthermore, many deeply inaccurate statements about JavaScript have been made. Frankly, I'm loosing confidence in this project as a whole. If the owners of this repo do not understand single threaded code, then rx.js is not ready for production.

trxcllnt commented 9 years ago

@Fresheyeball I don't feel warm and fuzzy about blocking the event loop either, but one of Rx's biggest selling points is its commitment to deterministic concurrency. If you tell the synchronous scheduler to schedule something three seconds from now, it doesn't have any other way to count time.

Fresheyeball commented 9 years ago

There is no such thing as a synchronous wait in JavaScript, because its single threaded. You can easily count time and wait three seconds with setTimeout. If what you are looking for synchronous code syntax for a wait, you just can't have that in JavaScript. That is why things like promises exist. (If developers who come from multithreaded languages, don't like that, tough. No one does, but its the way JavaScript works, and this is a JavaScript library.)

There is no such thing as concurrency in JavaScript, so I'm not sure why you would be committed to it, or consider it a selling point.

That said, can you provide a concrete example (unit test) of this? Something where this busy wait is absolutely required for a piece of code to execute deterministically? I read the unit tests for immediate scheduler and it was not there.

jdegoes commented 9 years ago

The word "concurrent" is being abused in this thread.

Please be assured that in Javascript, there will be no concurrent behavior regardless of whether you delay with spin-looping or timeouts. The code pasted above is worse than useless — spin-looping like that without so much as a yield has catastrophic implications for CPU usage.

I understand the motivation might be abstraction: providing both "now" and "later" implementations of the same interface, so code can choose scheduling behavior. But the fact that you can't do that without a spin-loop means the abstraction is broken or the motivation is wrong. Take your pick.

mattpodwysocki commented 9 years ago

@jdegoes the alternate is to what? Throw an error saying you cannot use this method to schedule something in the future with either the immediate or current thread scheduler? I'm ok with doing that as that's the way we did it in the past until someone requested that we had blocking semantics. I guess I can't make everyone happy here.

mattpodwysocki commented 9 years ago

@Fresheyeball the hyperbole in your comments isn't helpful here. Like I said, there are a couple of options:

  1. Throw an error saying you shouldn't use Scheduler.immediate and Scheduler.currentThread to schedule future actions
  2. Use setTimeout for future actions on Scheduler.immediate and Scheduler.currentThread which would be incorrect, because it wouldn't be "immediate"
  3. Keep the spin lock which locks the CPU

So, what would you have me do here? We already have schedulers which handle this just fine in Rx.Scheduler.timeout which uses setTimeout and is the default for any operator that introduces any level of asynchronicity, eg debounce, throttle, bufferWithTime, bufferWithTimeOrCount, etc. That and with RxJS-DOM, we have Scheduler.requestAnimationFrame, for animations. Choose the right one for what you're trying to do.

jdegoes commented 9 years ago

I would suggest there are several ways to solve the problem:

  1. Carving out two interfaces here. The scheduleRelative function does not make any sense with the "immediate" implementation since the only possible implementation is a spin-loop. It's possible there could exist a scheduler and a delayed scheduler (an extension of scheduler), and some code would require a delayed scheduler because it requires the additional capability of scheduleRelative.
  2. Failing that, I do think throwing an exception would be preferable. An exception is an admission that there are multiple capabilities which cannot always be implemented together (and which should therefore be factored out into separate interfaces, as above). But still, if there can exist only one scheduler interface for other reasons, an exception is better than a bad implementation.

I can easily see someone requesting the blocking implementation, but it's more than likely because they do not understand the Javascript execution model, and I would be astounded if such code survived to production.

trxcllnt commented 9 years ago

@jdegoes @Fresheyeball

I don't think anybody's suggested other code-paths continue to execute while the interpreter is hung on this loop.

I hear you both, and I understand where you're coming from. I believe our differences lie in how we each understand the role of Schedulers in RxJS, and the meaning of concurrency in a single-threaded context.

You're both right; concurrency is usually synonymous with a threading model, but single-threaded environments still have concurrency problems.

Schedulers are intended to coordinate both a. the time when actions are executed, and b. that the actions are executed in a specific order. Each Scheduler implements a unique scheduling strategy, and the differences between them are very important.

The CurrentThreadScheduler enqueues and executes actions in a FIFO queue, whereas the ImmediateScheduler recursively executes actions. In other words, the execution order of the CurrentThreadScheduler is breadth-first, and the ImmediateScheduler is depth-first.

Each scheduling strategy has its strengths and weaknesses, though CurrentThreadScheduler is probably the best choice most of the time. However, some tasks are best scheduled on the ImmediateScheduler, like synchronous nested switchLatests or depth-first expands.

Each scheduler must maintain its scheduling characteristics, regardless of when an action is scheduled to execute. Switching both schedulers to setTimeout's FIFO queue for future actions non-deterministically and unacceptably alters the ImmediateScheduler's behavior. It would break existing code and introduce race conditions that aren't possible to replicate.

_This is a very bad idea._

Correctness is the only option here, even if we have to block the UI thread to do it.

I threw this example together to illustrate what I mean. @mattpodwysocki: I actually found a bug in ImmediateScheduler's relative time calculation while I was doing it. Presently, ImmediateScheduler's loop only hangs if you call scheduleRelative with an absolute time [larger than scheduler.now()].

I fixed that bug and ended up with the output at the top of the gist. If you run this code in node for yourself, you'll see the order of the last two sequences (blocking-recursive a and b) don't match their synchronous counterparts (recursive a and b) above: https://gist.github.com/trxcllnt/1c277f25d2c0250f4ac3

Fresheyeball commented 9 years ago

I would be accepting of solutions 1 or 2.

trxcllnt commented 9 years ago

@Fresheyeball You've been told about the right way to schedule future actions without blocking, so Rx clearly isn't preventing you from accomplishing anything. The behavior you've reported is a feature, not a bug. If you do ever encounter a situation where the synchronous schedulers block you on future actions, Rx makes it exceedingly easy to swap out the concurrency model. Cross-communicating node processes and web workers are examples of threaded concurrency in JavaScript which Rx also has to take into account.

@mattpodwysocki Motion to close this issue.

mattpodwysocki commented 9 years ago

@trxcllnt @Fresheyeball We have "fixed" the issue in that it will throw an error if you try to schedule something in the future on the Immediate or CurrentThread schedulers

Fresheyeball commented 9 years ago

Vunderbar! Can I get a link to the Diff that documents this change?