Cancel scheduled timeout work, if no longer needed #2135

jayphelps · 2016-11-16T00:47:09Z

This ties the action subscription returned from a call to schedule() to the lifespan of the subscriber, so it handles error/complete. On reschedules it more explicitly cancels, if there's an existing.

I think this fixes it, but to be honest, I can't be sure. It's not immediately clear how to test it. At first glance, it doesn't appear we're testing this possible bug in the other operators which could have similar problems. e.g. debounceTime. So other operators could be hiding the fact that they don't clean up properly, because of guards in their work callback, or the subscriptions that protect against values continuing after error/complete.

I tried peeking into the internals of TestScheduler to see if maybe I could grab a reference to the VirtualAction and check action.pending, but because it's entirely virtual, the action comes and goes only inside a testScheduler.flush().

Perhaps @trxcllnt aka Mr. Scheduler can halp?

mattpodwysocki · 2016-11-16T00:49:00Z

@jayphelps perhaps there's a test we can write for this? I don't see anything here to that regard but should be.

jayphelps · 2016-11-16T00:49:26Z

@mattpodwysocki see my above post 😆

mattpodwysocki · 2016-11-16T01:18:07Z

@jayphelps * sigh * reading is fundamental...

trxcllnt · 2016-11-16T02:45:42Z

src/operator/timeout.ts

@@ -45,6 +46,8 @@ class TimeoutOperator<T> implements Operator<T, T> {
 class TimeoutSubscriber<T> extends Subscriber<T> {
  private index: number = 0;
  private _previousIndex: number = 0;
+  private action: Subscription = null;
+
  get previousIndex(): number {


If we're tracking the scheduled action subscription, do we still need previousIndex etc.? Same question for hasCompleted.

not sure, but hasCompleted indeed seems extraneous. I couldn't figure out why there is an index (aka count) check anyway? Why would it ever not be equal?

trxcllnt · 2016-11-16T02:54:13Z

src/operator/timeout.ts

+    const currentIndex = this.index;
+    const timeoutState = { subscriber: this, index: currentIndex };
+
+    this.cancelTimeout();


So here it seems like we can do this:

const { action } = this; if (!action) { this.add(this.action = this.scheduler.schedule(...)); } else { action.schedule(this.waitFor, timeoutState); }

Instead of scheduling an action, throwing it away, and scheduling again, we can recycle the Action and reschedule it for a new due time. This should be more efficient, and allow us to remove more code from this file.

👍 I'm on board with reusing the action.

and allow us to remove more code from this file.

What other code could be removed?

trxcllnt · 2016-11-16T03:07:44Z

@jayphelps we can use lift! If the changes in my review pan out, something like this should do the trick:

it('should unsubscribe from the scheduled timeout action when timeout is unsubscribed early', () => {
  const e1 =   hot('--a--b--c---d--e--|');
  const e1subs =   '^         !        ';
  const expected = '--a--b--c--        ';
  const unsub =    '          !        ';

  const result = e1
    .lift(function(source) {
      const timeoutSubscriber = this;
      const { action } = timeoutSubscriber; // get a ref to the action here,
      timeoutSubscriber.add(() => {         // because it'll be null by the
        if (!action.canceled) {             // time we get into this function.
          throw new Error('TimeoutSubscriber scheduled action wasn\'t canceled');
        }
      });
      return source._subscribe(timeoutSubscriber);
    })
    .timeout(50, null, rxTestScheduler);

  expectObservable(result, unsub).toBe(expected);
  expectSubscriptions(e1.subscriptions).toBe(e1subs);
});

coveralls · 2016-11-16T03:51:05Z

Coverage decreased (-0.008%) to 97.677% when pulling d34cb57 on jayphelps:timeout into e0cf882 on ReactiveX:master.

trxcllnt · 2016-11-16T06:05:08Z

@jayphelps I submitted a PR to your timeout branch.

…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 · 2016-11-29T08:56:21Z

@jayphelps I fixed the lint, rebased, tried and failed to push straight to this PR ref (GH doesn't like that), so I re-PR'd to your timeout branch.

fixes ReactiveX#2134

…heir 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.

…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.

coveralls · 2016-11-29T18:33:17Z

Coverage decreased (-0.008%) to 97.678% when pulling 0de6ef5 on jayphelps:timeout into f51b8f9 on ReactiveX:master.

benlesh · 2016-12-07T18:44:51Z

package.json

@@ -89,7 +89,7 @@
    "prepublish": "shx rm -rf ./typings && typings install && npm run build_all",
    "publish_docs": "./publish_docs.sh",
    "test_mocha": "mocha --opts spec/support/default.opts spec-js",
-    "debug_mocha": "node-debug _mocha --opts spec/support/debug.opts spec-js",
+    "debug_mocha": "node --inspect --debug-brk ./node_modules/.bin/_mocha --opts spec/support/debug.opts spec-js",


This change seems unrelated although I agree with it.

@trxcllnt I believe this is you, cool for me to remove?

as long as it's in a follow up PR that's fine with me. I didn't want to clutter up the PR list with internal config changes that only affect the devs working on the project, but I leave it up to your discretion

benlesh · 2016-12-07T18:48:10Z

spec/operators/timeout-spec.ts

+    const unsub =    '          !        ';
+
+    const result = e1
+      .lift(function(source) {


I'm not sure this test is very straightforward. Perhaps it could be refactored into using the let operator, and returning a more basic observable

can we pair on this? I couldn't figure out how to test this at all, so @trxcllnt came up with this solution and it's still not clear to me how we could do it better. 💃

@Blesh let gives you a handle on the source Observable, but lift gives you a handle to each subscriber. In this case we want to know whether the subscriber did something (canceled its internal scheduled action subscription), so we use lift. textbook use-case for lift.

@trxcllnt ugh.. this is a case where we're abusing the fact that Operator has a call method, and we're therefor passing a function to lift here? This is really convoluted, it took me a while to figure that out. If anything this test reminded me why Operator probably shouldn't have a call method.

I think we need to find a different way to test this.

@jayphelps I think the way to test this, and other things like it, would be to add functionality to the TestScheduler so that it tracks the Actions it's created and those Actions can be examined to see at what tick they were cancelled. For now, we can leave these tests in here, but they'll need comments explaining what they're doing and why it works.

Basically we need comments explaining why lift is magically accepting a function here and not an Operator instance.

We could simply pass an object with a call method.

.lift({ call(source) { ... } })

@Blesh this is exactly why Operator has a call method. In an ideal world an operator would just be a function, but because JS sucks, we've had to make them classes. See RxJava, where Operators are functions.

Ben is OK with this for now, but wants some inline comments about why we're doing

benlesh · 2016-12-07T18:48:28Z

spec/operators/timeoutWith-spec.ts

+    const unsub =      '              !    ';
+
+    const result = e1
+      .lift(function(source) {


Same as above

benlesh

Basically we need comments explaining why lift is magically accepting a function here and not an Operator instance.

benlesh · 2016-12-11T05:42:36Z

spec/operators/timeout-spec.ts

+    const unsub =    '          !        ';
+
+    const result = e1
+      .lift(function(source) {


@jayphelps I think the way to test this, and other things like it, would be to add functionality to the TestScheduler so that it tracks the Actions it's created and those Actions can be examined to see at what tick they were cancelled. For now, we can leave these tests in here, but they'll need comments explaining what they're doing and why it works.

trxcllnt · 2016-12-12T02:40:21Z

@Blesh alternatively, we should be able to define Operator as a hybrid type (so it's both an Object with a call method, and a Function with thisArg: Subscriber<T>), but my TypeScript-foo was not strong enough to defeat the compiler. maybe @david-driscoll or @kwonoj could help?

trxcllnt · 2016-12-20T22:32:43Z

@Blesh what are the blockers to merging this? @jayphelps if possible, could you allow adding commits to this PR from other collaborators?

jayphelps · 2016-12-20T22:35:06Z

@trxcllnt the edit bit is already enabled:

We discussed this PR today, minor changes needed: https://github.com/ReactiveX/rxjs-core-notes/blob/master/2016-12/december-20.md#review-prsissues

Needs comments on why we're using lift this way, also remove npm script changes.

I won't be able to get to it for at least a couple days, but others with edit bit are welcome to modify.

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

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

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 ReactiveX#2347

…guments Emit undefined insteady of empty array by resulting Observable, when callback function is called without success parameters. Closes ReactiveX#2254

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

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.

- closes ReactiveX#2372

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

…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 changes

fix commit messages

kwonoj · 2017-02-15T23:54:23Z

I'm going to lower commit message checker to warning so ignore failure for now please.

benlesh · 2017-02-16T01:31:07Z

LOL... why are there so many commits in this PR now?

trxcllnt · 2017-02-16T05:51:31Z

@Blesh lint/spec typings fixes and merge commits/conflicts le sigh

kwonoj · 2017-02-16T05:56:25Z

@trxcllnt sorry for inconvenience but rebase to master once again will resolve commit message checker failure (though you'll see warning still).

benlesh · 2017-02-21T03:36:01Z

Ugh. I'm not sure what to do with this PR. It's older that dirt. It's a solid fix, but there's something crazy going on with the commits. Can we resubmit this one and/or clean it up?

jayphelps · 2017-02-21T03:42:33Z

hmm I'm not really following all of @trxcllnt's additions. If he's not available to fix, we can abandon this PR and I can try to fix the underlying problem again from a fresh start?

trxcllnt · 2017-02-21T04:23:31Z

@jayphelps @Blesh they're just merge commits from master to resolve the merge conflicts. the CI was being really restrictive against merge commits, but @kwonoj relaxed that requirement in master, so I've done one more follow-up merge to hopefully make travis pass.

merge master again

coveralls · 2017-02-21T04:35:08Z

Coverage decreased (-0.008%) to 97.681% when pulling 9e16f48 on jayphelps:timeout into 6ce4773 on ReactiveX:master.

lock · 2018-06-06T12:41:03Z

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.

trxcllnt reviewed Nov 16, 2016

View reviewed changes

jayphelps added the PR: DO NOT MERGE YET PLZ label Nov 17, 2016

jayphelps force-pushed the timeout branch from 8049b78 to 1761f59 Compare November 20, 2016 20:29

PatrickJS mentioned this pull request Nov 23, 2016

Bump rxjs to 5.0.0-rc.2 angular/angular#12476

Closed

3 tasks

jayphelps and others added 5 commits November 29, 2016 10:04

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

9441def

fixes ReactiveX#2134

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

9763c03

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

6d4937f

jayphelps force-pushed the timeout branch from 1761f59 to 0de6ef5 Compare November 29, 2016 18:18

jayphelps mentioned this pull request Nov 29, 2016

Pr 2135 jayphelps/rxjs#2

Closed

jayphelps removed the PR: DO NOT MERGE YET PLZ label Nov 29, 2016

benlesh requested changes Dec 7, 2016

View reviewed changes

benlesh previously requested changes Dec 11, 2016

View reviewed changes

mmcgahan mentioned this pull request Feb 1, 2017

MW 622 hanging integration meetuparchive/meetup-web-platform#150

Merged

mpodlasin and others added 12 commits February 15, 2017 15:42

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

f38a27e

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

feat(webSocket): Add binaryType to config object

db72756

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

chore(danger): update dangerfile to validate commit message

2b66fc4

fix(bindNodeCallback): emit undefined when callback has no success ar…

0bbfdf2

…guments Emit undefined insteady of empty array by resulting Observable, when callback function is called without success parameters. Closes ReactiveX#2254

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

dc568da

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

74dd235

- closes ReactiveX#2372

feat(windowTime): maxWindowSize parameter in windowTime operator

b1bb0ce

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

fix(timeoutWith): update timeoutWith to work with new Subscriber leak…

790a469

… fix changes

Merge pull request #6 from trxcllnt/jayphelps-timeout

0f2522d

fix commit messages

trxcllnt added 2 commits February 20, 2017 20:28

Merge branch 'master' into jayphelps-timeout

94bee5d

Merge pull request #7 from trxcllnt/jayphelps-timeout

c9994b0

merge master again

benlesh added the Next Patch Version label Mar 27, 2017

benlesh approved these changes Mar 27, 2017

View reviewed changes

kwonoj approved these changes Mar 27, 2017

View reviewed changes

benlesh merged commit 3e9d529 into ReactiveX:master Mar 27, 2017

This was referenced Nov 15, 2017

Discuss the value of lift in the 6.0 world #2911

Closed

Refactor Operator classes to be just functions? #3057

Closed

lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018

Cancel scheduled timeout work, if no longer needed #2135

Cancel scheduled timeout work, if no longer needed #2135

Conversation

jayphelps commented Nov 16, 2016 • edited Loading

mattpodwysocki commented Nov 16, 2016

jayphelps commented Nov 16, 2016 • edited Loading

mattpodwysocki commented Nov 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trxcllnt Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trxcllnt commented Nov 16, 2016 • edited Loading

coveralls commented Nov 16, 2016 • edited Loading

trxcllnt commented Nov 16, 2016 • edited Loading

trxcllnt commented Nov 29, 2016

coveralls commented Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trxcllnt Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trxcllnt commented Dec 12, 2016 • edited Loading

trxcllnt commented Dec 20, 2016

jayphelps commented Dec 20, 2016

kwonoj commented Feb 15, 2017

benlesh commented Feb 16, 2017

trxcllnt commented Feb 16, 2017

kwonoj commented Feb 16, 2017

benlesh commented Feb 21, 2017

jayphelps commented Feb 21, 2017 • edited Loading

trxcllnt commented Feb 21, 2017 • edited Loading

coveralls commented Feb 21, 2017 • edited Loading

lock bot commented Jun 6, 2018

jayphelps commented Nov 16, 2016 •

edited

Loading

jayphelps commented Nov 16, 2016 •

edited

Loading

trxcllnt Nov 16, 2016 •

edited

Loading

trxcllnt commented Nov 16, 2016 •

edited

Loading

coveralls commented Nov 16, 2016 •

edited

Loading

trxcllnt commented Nov 16, 2016 •

edited

Loading

coveralls commented Nov 29, 2016 •

edited

Loading

trxcllnt Dec 12, 2016 •

edited

Loading

trxcllnt commented Dec 12, 2016 •

edited

Loading

jayphelps commented Feb 21, 2017 •

edited

Loading

trxcllnt commented Feb 21, 2017 •

edited

Loading

coveralls commented Feb 21, 2017 •

edited

Loading