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

.timeout doesn't cancel setTimeout #2134

Closed
PatrickJS opened this issue Nov 15, 2016 · 20 comments · Fixed by #2135
Closed

.timeout doesn't cancel setTimeout #2134

PatrickJS opened this issue Nov 15, 2016 · 20 comments · Fixed by #2135

Comments

@PatrickJS
Copy link
Contributor

RxJS version:
rxjs beta.12
Code to reproduce:

Expected behavior:
cancel the internal setTimeout when it's not needed anymore (after first response)

Actual behavior:
setTimeout still resolves which delays universal since zone.js tracks it
Additional information:

Angular Universal uses zone.js to keep track of async calls before rendering. There seems to be a problem with .timeout on an http call
here's a repro
https://github.com/gdi2290/universal-starter-rxjs-timeout-issue/blob/master/src/app/shared/api.service.ts#L23
npm run watch:dev
comment out that line
there are some console logs on the server that show the stable time taking 2000ms with the timeout rather than less than 1ms

@kwonoj
Copy link
Member

kwonoj commented Nov 15, 2016

Please allow me quick question to understand this - so is this about cancelling scheduled timeout per each emit as soon as value emits?

@PatrickJS
Copy link
Contributor Author

@kwonoj yup

@kwonoj
Copy link
Member

kwonoj commented Nov 15, 2016

as I have barely no experience with zone.js I can't say for this really sure, so I'll leave this open and /cc @Blesh and @jayphelps . Personally this sounds more of issue of zone.js though. If this is behavior of zone, it might possible other codebase in Rx which rely on scheduled action until next value emit might possibly being affected as well.

@david-driscoll
Copy link
Member

I think (personally) it's fair that if the destination observable is unsubscribed, that any pending setTimeout/setInterval should also be cancelled.

this.scheduler.schedule(TimeoutSubscriber.dispatchTimeout, this.waitFor, { subscriber: this, index: currentIndex });

I think in this scenario, it may be as simple as adding the Action to the destination subscriber.

ie...

this.scheduler.schedule(TimeoutSubscriber.dispatchTimeout, this.waitFor, { subscriber: this, index: currentIndex });

// becomes

this.destination.add(this.scheduler.schedule(TimeoutSubscriber.dispatchTimeout, this.waitFor, { subscriber: this, index: currentIndex }));

Makes me wonder how buffer / window behave.

@kwonoj
Copy link
Member

kwonoj commented Nov 15, 2016

@david-driscoll but I assume those will not solve this specific issue since issue is about cancelling timeout per-each-value-emit. am I understand correctly?

@PatrickJS
Copy link
Contributor Author

PatrickJS commented Nov 15, 2016

Let me reword it with another example just to be sure everyone understands the problem we ran into. Users are currently following this pattern as a workaround for .timeout which isn't correct (commented).

Observable.race(
  this.http.get(url),
  Observable.of(null).delay(2000) // I would prefer to return immediately if the other observables in the race are done/errored/complete
)
.subscribe(data => this.data = data)

The idea is to cancel the http observable if it exceed the timeout limit otherwise if the http observable first then cancel the timeout (in this case the internal setTimeout).

For zone.js etc in Angular Universal, we are using zone.js to track when the application is stable before rendering the Application. The problem here happens when the setTimeout is still resolving even if the http call is done which prevents rendering

@david-driscoll
Copy link
Member

I would think that if a timeout subscriber has been cancelled / completed that any pending timeouts should be cancelled. So that we clean up after ourselves.

What zone.js does is simply wraps anything async in the browser, timeouts, promises, etc. Using the zone.js api it's then possible to see if there are any scheduled tasks at a given time.

For this problem, there is always an outstanding task when timeout is subscribed to, which never gets cancelled, so zone.js will see a pending change for 2000ms regardless of the actual completion of the event.

My code changes are incorrect, we would need to keep track of the base subscription, and unsubscribe in _next, _error and _complete. Additionally hasCompleted would no longer be needed as dispatchTimeout would only be called the timeout actually happened.

I'll leave it out for @Blesh and @jayphelps to chime in on additionally.

@PatrickJS
Copy link
Contributor Author

@david-driscoll yup you said it perfectly 👍

@jayphelps
Copy link
Member

jayphelps commented Nov 15, 2016

A bit pedantic, but just for clarity, we actually use setInterval to schedule async things, even if they only occur once.

Edit: the following is wrong. Schedulers do support cancellation.

We don't currently provide any mechanism to cancel scheduled work. So every operator which schedules, that work will be invoked regardless of whether or not anyone is subscribing anymore. That work then double checks whether or not it should proceed, e.g. checking source.hasCompleted. Presumably, there are other operator usages that will prevent ng2 from flushing as soon as it could.

Certainly an argument could be made that we should provide a cancellation mechanism. A quick look at v4 suggests it did. No guarantees, but this might make our operator code cleaner and less error prone as well, since the work callback wouldn't need to check whether or not the source has completed I think.

Cc/ @trxcllnt

@mattpodwysocki
Copy link
Collaborator

@jayphelps you are correct, it did, which should be the default behavior to always tear down upon cancellation/completion and not just removing the item from the queue.

@trxcllnt
Copy link
Member

@jayphelps We don't currently provide any mechanism to cancel scheduled work. So every operator which schedules, that work will be invoked regardless of whether or not anyone is subscribing anymore.

This is not the case. Canceling an AsyncAction before the due time will always clear the scheduled interval, full stop. Like @david-driscoll has pointed out, the timeout operator needs to clean up after itself.

@jayphelps
Copy link
Member

@trxcllnt OH. Can you show where this is done? I only see a single clearInterval and do not see how it could be hit for anything but a reschedule on a different time.

@trxcllnt
Copy link
Member

@jayphelps here's the line that does it. To see it in action, drop this into esnextb.in:

import { Scheduler } from 'rxjs';

const subscription = Scheduler.async.schedule(function() {
  console.log('scheduled action executed');
}, 2000);

setTimeout(() => {
  console.log('canceling scheduled action');
  debugger
  subscription.unsubscribe();
}, 1000);

Since all scheduler actions extend the AsyncAction, this behavior is consistent across the schedulers.

@jayphelps
Copy link
Member

@trxcllnt 👍 ahhh okay. Missed that. My brain couldn't see past the fact that it's called recycle 😜

@trxcllnt
Copy link
Member

trxcllnt commented Nov 15, 2016

@jayphelps the schedulers were a particular challenge. Attempting to take advantage of the more precise interval execution via setInterval vs. serial setTimeouts introduced a number of edge cases the schedulers have never encountered before. Additionally, we'd like to maximize code reuse, so AsyncAction needs an API that can be minimally extended to support asap, animationFrame, and virtual time/test scheduling. And last but not least, we need it to be speedy and lightweight.

I'll be the first one to admit the scheduler code is clever (in the :/ way), but it was forged through the flames of rigorous speed and memory tests, and I'm resigned to the idea that sometimes the code you need isn't the code you want. That said, I'm 100% on board for contributions and collaborations that improve quality, speed, etc. and happy to pair or answer questions for anybody who is interested in digging in.

@trxcllnt
Copy link
Member

trxcllnt commented Nov 15, 2016

@jayphelps ...it's called recycle

Heh, I totally understand. Naming is hard. My thought process was more, sometimes recycling means you reuse the object, other times you dispose of it. ;j

@trxcllnt
Copy link
Member

Oh, not to mention async (or asap/queue/animationFrame/virtual) actions can all reschedule themselves with different due times per iteration of the work. One of the biggest improvements (imo) to the scheduling architecture is that repeatedly scheduled work (for example, on an interval) only allocates a single Subscription. Obviously doing this and still supporting all the behaviors Rx has always supported is, as evident by this discussion, complicated.

jayphelps added a commit to jayphelps/rxjs that referenced this issue Nov 16, 2016
trxcllnt added a commit to trxcllnt/rxjs that referenced this issue Nov 16, 2016
…ed timeout actions.

The timeout and timeoutWith operators should dispose their scheduled timeout actions on

unsubscription. Also, given the new scheduling architecture, they can recycle their scheduled

actions so just one action is allocated per subscription.

ReactiveX#2134 ReactiveX#2135
trxcllnt added a commit to trxcllnt/rxjs that referenced this issue Nov 20, 2016
…ed timeout actions.

The timeout and timeoutWith operators should dispose their scheduled timeout actions on

unsubscription. Also, given the new scheduling architecture, they can recycle their scheduled

actions so just one action is allocated per subscription.

ReactiveX#2134 ReactiveX#2135
jayphelps pushed a commit to jayphelps/rxjs that referenced this issue Nov 20, 2016
…ed timeout actions.

The timeout and timeoutWith operators should dispose their scheduled timeout actions on

unsubscription. Also, given the new scheduling architecture, they can recycle their scheduled

actions so just one action is allocated per subscription.

ReactiveX#2134 ReactiveX#2135
jayphelps added a commit to jayphelps/rxjs that referenced this issue Nov 20, 2016
jayphelps pushed a commit to jayphelps/rxjs that referenced this issue Nov 20, 2016
…ed timeout actions.

The timeout and timeoutWith operators should dispose their scheduled timeout actions on

unsubscription. Also, given the new scheduling architecture, they can recycle their scheduled

actions so just one action is allocated per subscription.

ReactiveX#2134 ReactiveX#2135
trxcllnt pushed a commit to trxcllnt/rxjs that referenced this issue Nov 29, 2016
jayphelps added a commit to jayphelps/rxjs that referenced this issue Nov 29, 2016
@xzengCB
Copy link

xzengCB commented Dec 20, 2016

I met the same issue when trying to set global timeout for http request

@trxcllnt
Copy link
Member

ping @Blesh @jayphelps

benlesh pushed a commit that referenced this issue Mar 27, 2017
* fix(timeout): Cancels scheduled timeout, if no longer needed

fixes #2134

* fix(timeoutWith): Cancels scheduled timeout, if no longer needed

* build(npm-scripts): update debug_mocha npm script for node 6

* fix(VirtualAction): Block rescheduled VirtualActions from executing their scheduled work.

VirtualActions are immutable so they can be inspected by the TestScheduler. In order to mirror

rescheduled stateful Actions, rescheduled VirtualActions shouldn't execute if they've been

rescheduled before execution.

* fix(timeout): Update timeout and timeoutWith to recycle their scheduled timeout actions.

The timeout and timeoutWith operators should dispose their scheduled timeout actions on

unsubscription. Also, given the new scheduling architecture, they can recycle their scheduled

actions so just one action is allocated per subscription.

* test(timeout): Add types to timeout and timeoutWith specs

* Fix merge conflicts

* Fix timeoutWith to work with new Subscriber leak fix.

* fix(timeout-spec): fix merge conflicts

* fix(Subscription): fold ChildSubscription logic into Subscriber to prevent operators from leaking ChildSubscriptions. (#2360)

The addition of ChildSubscription to fix #2244 accidentally introduced a different memory leak. Most operators that add and remove inner Subscriptions store the inner Subscriber instance, not the value returned by Subscription#add. When they try to remove the inner Subscription manually, nothing is removed, because the ChildSubscription wrapper instance is the one added to the subscriptions list.

Fixes #2355

* chore(publish): 5.1.1

* Ignore coverage

It's 5.5mb that people installing this don't need :)

* feat(AjaxObservable) : support 'PATCH' request type

Add support of the 'PATCH' request type based on the already existing 'PUT' request.

* fix(subscribeToResult): accept array-like as result

Accept array-like as a result to subscribe, so that
Observable.from and operators using subscribeToResult
have identical behaviour.

* chore(ajax.patch): Adds test for ajax.patch

* fix(forkJoin): add type signature for single observable with selector

Add type signature for using forkJoin with single observable as first parameter
and selector function as second parameter, so that typings would not prevent
usage which is permitted and properly handled by operator.

Closes #2347

* feat(webSocket): Add binaryType to config object

Add binaryType to config object, so that it is possible
to set that parameter on underlying socket before any
data emits happen.

Closes #2353

* fix(merge): return Observable when called with single lowerCaseO

Return Observable when merge is called with single lower case observable,
so that merge would always return Observable instance.

* fix(bindNodeCallback): emit undefined when callback has no success arguments

Emit undefined insteady of empty array by resulting Observable,
when callback function is called without success parameters.

Closes #2254

* chore(danger): update dangerfile to validate commit message

* chore(*): correctly scope disabled `max-line-length` tslint rule

The max line length is set to 150 in 'tslint.json'. In specific regions, it is
desirable to allow longer lines, so these regions should be wrapped in comments
like the following:

```js
// Max line length enforced here.

/* tslint:disable:max-line-length */
// Max line length NOT enforced here.
/* tslint:enable:max-line-length */   <-- CORRECT

// Max line length enforced here.
```

In many cases, the re-enabling comment incorrectly included `disable` instead of
`enable` (as shown below), which essentially keeps the `max-line-length`
**disabled** for the rest of the file:

```js
// Max line length enforced here.

/* tslint:disable:max-line-length */
// Max line length NOT enforced here.
/* tslint:disable:max-line-length */   <-- INCORRECT

// Max line length NOT enforced here.
```

This commit fixes these comments, so the `max-line-length` rule is properly
enforced in regions that don't need longer lines.

* fix(bindCallback): emit undefined when callback is without arguments

In resulting Observable emit undefined when callback is called without
parameters, instead of emitting empty array.

* fix(mergeAll): introduce variant support <T, R> for mergeMap

- closes #2372

* feat(windowTime): maxWindowSize parameter in windowTime operator

Adds new parameter in windowTime operator to control how much values given
window can emit.

Closes #1301

* docs(ObservableInput): add ObservableInput and SubscribableOrPromise descriptions

Add ObservableInput and SubscribableOrPromise interface descriptions,
as well as link these interfaces in type descriptions of operators,
so that users always know what kind of parameters they can pass to
used methods.

* fix(timeoutWith): update timeoutWith to work with new Subscriber leak fix changes
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants