Add option to reset interval on success #18

vially · 2020-03-17T15:44:56Z

This pull-request adds an option to reset the state of the retryBackoff operator on successful emissions on the source observable.

When this option is enabled (it's off by default to maintain backwards compatibility) both the delay interval and the attempt counter gets reset when the source observable emits a value.

Use cases

Some of the arguments and motivation for this option were nicely laid out in ReactiveX/rxjs#1413. A similar option was recently added to the retry operator (ReactiveX/rxjs#5280).

Quoting @benlesh from the linked issue:

Often with retryWhen there is a desire to reset some "state" of your retries when the stream your retrying starts receiving values successfully again. For example, in a network retry scenario where you want to exponentially step back from the first failure. Once you get a value over that network observable, you want to reset your exponential step back to the first level again, because things are going well now.

Code review notes

The first four commits are not related to this pull-request and I can submit them separately if needed. They are related to some small nuisances I encountered when running the tests locally.

alex-okrushko · 2020-04-03T19:03:31Z

Ah, sorry. Very nice!
I saw the initial issue, but got distracted.

Ben was just talking at ng-conf about reset for retry and I remembered the issue that you opened.

vially · 2020-04-03T19:09:35Z

No worries! Let me know if you'd prefer to submit the unrelated commits separately to keep this pull-request cleaner.

package.json

alex-okrushko · 2020-04-12T14:49:00Z

yarn.lock

@@ -2926,6 +2926,13 @@ run-queue@^1.0.0, run-queue@^1.0.3:
  dependencies:
    aproba "^1.1.1"

+rxjs@^6.4.0:


this would be reverted as well

alex-okrushko · 2020-04-12T15:01:23Z

src/operators/retryBackoff.ts

-  return <T>(source: Observable<T>) =>
-    source.pipe(
+  return <T>(source: Observable<T>) => {
+    let index = 0;


This would create a problem with referential transparency.
Nicholas Jamieson describes it very well here: https://stackoverflow.com/a/52022545/1167879

💯 Awesome catch and thanks for the link. Very interesting explanation of the perils of keeping state in operators. I also found this article, by the same author, which goes into a bit more details about the same problem.

These are the steps I took to fix this issue:

add two tests related to this internal state problems (296637b): ✔️ Tests passing
This was to confirm everything worked as expected before my changes to the operator.

add my initial naive implementation (d1356ab): ❌ Tests failing
One of those two tests was failing, which confirms the initial implementation was buggy. Although it wasn't the referential transparency one that was failing, it was the other one.

apply the fix suggested in stackoverflow/linked article (df20de5): ✔️ Tests passing

Very nice! Thanks for getting it done.

Let's adjust the tap and one more quick one - do you have prettier (extension) installed for formatting? I should've added it to dev deps as well 😕

Also, what are your thought about making resetOnSuccess = true by default?

I've reformatted the retryBackoff.ts file using this prettier config (because it seemed close enough to the original formatting):

{ "arrowParens": "avoid", "singleQuote": true }

Also, what are your thought about making resetOnSuccess = true by default?

My impression is that having it true by default is a reasonable default (I think more often than not that's what you want). My main concern is that it's a breaking change but that shouldn't be a problem if it will be introduced with a major version bump and documented accordingly.

alex-okrushko · 2020-04-12T15:09:46Z

src/operators/retryBackoff.ts

-      )
+      ),
+      tap((_: T) => {
+        if (resetOnSuccess) {


if you are not using arguments, then there's no need to put them at all :) () => {...}

Removing the argument completely makes TypeScript throw an error when compiling the code. Keeping the argument in there, even if it's not used, seemed to be a good trade-off in order to make the code compile.

I would still like to get rid of it as you've suggested but I haven't found a way to do so in a clean way. Let me know if you have any suggestions.

Hm... I don't see any complains from TS 🙂(did a git pull for you PR).

You're right, it seems to compile just fine now (8fa59d7). Not sure what has changed since I saw that error but it's good that it works now.

The mocha `--compilers` flag is deprecated in favor of `--require`: https://github.com/mochajs/mocha/wiki/compilers-deprecation

vially

Sorry for the late reply. Took me a while to find the time to get back to this to address your feedback.

vially · 2020-05-09T14:43:07Z

src/operators/retryBackoff.ts

-      )
+      ),
+      tap((_: T) => {
+        if (resetOnSuccess) {


Removing the argument completely makes TypeScript throw an error when compiling the code. Keeping the argument in there, even if it's not used, seemed to be a good trade-off in order to make the code compile.

I would still like to get rid of it as you've suggested but I haven't found a way to do so in a clean way. Let me know if you have any suggestions.

vially · 2020-05-09T15:13:51Z

src/operators/retryBackoff.ts

-  return <T>(source: Observable<T>) =>
-    source.pipe(
+  return <T>(source: Observable<T>) => {
+    let index = 0;


💯 Awesome catch and thanks for the link. Very interesting explanation of the perils of keeping state in operators. I also found this article, by the same author, which goes into a bit more details about the same problem.

These are the steps I took to fix this issue:

add two tests related to this internal state problems (296637b): ✔️ Tests passing
This was to confirm everything worked as expected before my changes to the operator.

add my initial naive implementation (d1356ab): ❌ Tests failing
One of those two tests was failing, which confirms the initial implementation was buggy. Although it wasn't the referential transparency one that was failing, it was the other one.

apply the fix suggested in stackoverflow/linked article (df20de5): ✔️ Tests passing

vially · 2020-05-16T11:50:59Z

I've added two more commits to address your latest feedback. I can squash and clean-up the commits before merging but I didn't want to do it yet because it makes it harder to review just the new changes.

alex-okrushko · 2020-05-17T19:27:08Z

Awesome @vially !
Thanks a lot for your contribution and for following through all the review changes.

Announced it as well: https://twitter.com/AlexOkrushko/status/1262202129772621824

Add build directory to gitignore

a5f644b

alex-okrushko requested changes Apr 12, 2020

View reviewed changes

vially and others added 5 commits May 9, 2020 16:25

Fix ts-node test compilation error

bbc09f0

Replace compilers flag with require

e468e16

The mocha `--compilers` flag is deprecated in favor of `--require`: https://github.com/mochajs/mocha/wiki/compilers-deprecation

Add tests related to internal state

296637b

Add resetOnSuccess option to retryBackoff operator

d1356ab

Fix referential transparency issue

df20de5

vially force-pushed the reset-on-success branch from f579e15 to df20de5 Compare May 9, 2020 14:35

vially commented May 9, 2020

View reviewed changes

vially added 2 commits May 16, 2020 13:19

fixup! Add resetOnSuccess option to retryBackoff operator

8fa59d7

Format retryBackoff with prettier

b34591f

alex-okrushko approved these changes May 17, 2020

View reviewed changes

alex-okrushko merged commit 7d38283 into alex-okrushko:master May 17, 2020

vially deleted the reset-on-success branch May 18, 2020 06:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to reset interval on success #18

Add option to reset interval on success #18

vially commented Mar 17, 2020

alex-okrushko commented Apr 3, 2020

vially commented Apr 3, 2020

alex-okrushko Apr 12, 2020

alex-okrushko Apr 12, 2020

vially May 9, 2020

alex-okrushko May 14, 2020

vially May 16, 2020

alex-okrushko Apr 12, 2020

vially May 9, 2020

alex-okrushko May 14, 2020

vially May 16, 2020

vially left a comment

vially May 9, 2020

vially May 9, 2020

vially commented May 16, 2020

alex-okrushko commented May 17, 2020 •

edited

Loading

Add option to reset interval on success #18

Add option to reset interval on success #18

Conversation

vially commented Mar 17, 2020

Use cases

Code review notes

alex-okrushko commented Apr 3, 2020

vially commented Apr 3, 2020

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

vially left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vially commented May 16, 2020

alex-okrushko commented May 17, 2020 • edited Loading

alex-okrushko commented May 17, 2020 •

edited

Loading