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

handleWith causes Future chain to be called twice if engage resolve fails. #44

Open
skeet70 opened this issue Sep 12, 2022 · 2 comments

Comments

@skeet70
Copy link
Member

skeet70 commented Sep 12, 2022

This is pretty edge case and strange behavior. This was discovered while using the library where the Future in question changed test state. When an assertion in the .engage on that future failed, it caused the handleWith code of the already engaged Future to be called again, which cause the map to be called again, which then blew up because the state had already changed. I pushed a minimal reproduction of the bug on the branch handle-called-out-of-scope.

test("does not get run if engages above this scope throw", (done) => {
            let mapCalledTimes = 0;
            let handleWithCalledTimes = 0;
            const action = Future.of(33)
                .handleWith((e) => {
                    console.log(`failed an infallible future: ${e}`);
                    handleWithCalledTimes++;
                    return Future.of(-1);
                })
                .map((r) => {
                    mapCalledTimes++;
                    return r;
                });

            try {
                action.engage(
                    (e) => {
                        throw e;
                    },
                    (r) => {
                        throw new Error(`oh no, something went wrong after the future has run to completion on ${r}`);
                    }
                );
            } catch (e) {
                expect(handleWithCalledTimes).toBe(0);
                expect(mapCalledTimes).toBe(1);
                done();
            }
        });

That's the test entirely though. The error thrown in the .engage causes the whole chain of the Future the engage was called on to run when it's somehow caught by the handleWith. The same thing happens if you start with a rejected Future, though then everything in the chain is run twice including the handleWith.

@skeet70
Copy link
Member Author

skeet70 commented Sep 12, 2022

It may be something specific to the interaction of engage and handleWith because the same test taking advantage of the Thennable on the Future doesn't have this problem.

test("does not get run if engages above this scope throw", async (done) => {
            let mapCalledTimes = 0;
            let handleWithCalledTimes = 0;
            const action = Future.of(33)
                .handleWith((e) => {
                    console.log(`failed an infallible future: ${e}`);
                    handleWithCalledTimes++;
                    return Future.of(-1);
                })
                .map((r) => {
                    mapCalledTimes++;
                    return r;
                });

            try {
                let r = await action;
                throw new Error(`oh no, something went wrong after the future has run to completion on ${r}`);
            } catch (e) {
                expect(handleWithCalledTimes).toBe(0);
                expect(mapCalledTimes).toBe(1);
                done();
            }
        });

@skeet70
Copy link
Member Author

skeet70 commented Sep 12, 2022

It does also have this problem if the reject side fails, which is something people are likely frequently doing.

test("does not get run if engages above this scope throw in the rejection", (done) => {
            let mapCalledTimes = 0;
            let handleWithCalledTimes = 0;
            const action: Future<Error, number> = Future.reject(new Error("error message"))
                .handleWith((e): any => {
                    console.log(`failed a future: ${e}`);
                    handleWithCalledTimes++;
                    return Future.of(-1);
                })
                .map((r) => {
                    console.log(`mapped`);
                    mapCalledTimes++;
                    return r;
                });

            try {
                action.engage(
                    (e) => {
                        throw e;
                    },
                    (r) => {
                        throw new Error(`oh no, something went wrong after the future has run to completion on ${r}`);
                    }
                );
            } catch (e) {
                expect(handleWithCalledTimes).toBe(1);
                expect(mapCalledTimes).toBe(1);
                done();
            }
        });

results in

  console.log
    failed a future: Error: error message

      at Future.test.ts:256:29

  console.log
    mapped

      at Future.test.ts:261:29

  console.log
    failed a future: Error: oh no, something went wrong after the future has run to completion on -1

      at Future.test.ts:256:29

  console.log
    mapped

      at Future.test.ts:261:29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant