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

Node >= 8.0.0 inconsistently not throwing "Maximum call stack size exceeded" #17684

Closed
remojansen opened this issue Dec 14, 2017 · 5 comments
Closed

Comments

@remojansen
Copy link

remojansen commented Dec 14, 2017

Build system information
Build language: node_js
Build group: stable
Build dist: trusty
Build id: 316498514
Job id: 316498516
Runtime kernel version: 4.9.6-040906-generic
travis-build version: 724aa1721
Build image provisioning date and time
Tue Dec  5 20:11:19 UTC 2017
Operating System Details
Distributor ID:	Ubuntu
Description:	Ubuntu 14.04.5 LTS
Release:	14.04
Codename:	trusty

I'm the author of InversifyJS. InversifyJS is an IoC container and it is designed to throw an exception if it finds a circular dependency.

try {
    const result = willThrow();
    throw new Error(
        `This line should never be executed. Expected 
        'willThrow' to throw! ${JSON.stringify(result)}`
    );
} catch (e) {
    // expect e to match some error (not the error above)
}

The willThrow function should catch an Error of type Maximum call stack size exceeded and throw a custom error this is the case in versions 7.10.1, 8.8.1 but not the case in 9.2.0 or 9.3.0.

It is quite crazy because it is resolving an object that cannot be resolved (a circular object).

I have tried 9.3.0 in a few machines:

  • Ubuntu 14.04.5 LTS (Circle CI)
  • Windows 8.1 x86 (App veyor)
  • Macbook Pro 16.6.0 Darwin Kernel Version 16.6.0: Fri Apr 14 16:21:16 PDT 2017; root:xnu-3789.60.24~6/RELEASE_X86_64 x86_64
  • Linux Mint 4.4.0-21-generic 37-Ubuntu SMP Mon Apr 18 18:33:37 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux

The only machines with the expected behavior (an exception is thrown) are the Macbook Pro and the WIndows machine.

I have a build that reproduces the issue https://travis-ci.org/inversify/InversifyJS/builds/316498439 and a PR inversify/InversifyJS#716.

Thanks

@remojansen remojansen changed the title Node 9.3.0 not throwing "Maximum call stack size exceeded"? Node > 9.2.0 not throwing "Maximum call stack size exceeded"? Dec 14, 2017
@addaleax
Copy link
Member

This seems to be flaky on earlier versions of Node (at least 9.1.0) as well.

Is there any chance you could provide a minimal reproduction using non-transpiled code?

@remojansen
Copy link
Author

I will try but I'ven unable to reproduce it using minimal code so far.

@remojansen remojansen changed the title Node > 9.2.0 not throwing "Maximum call stack size exceeded"? Node > 9.1.0 not throwing "Maximum call stack size exceeded"? Dec 15, 2017
@remojansen
Copy link
Author

remojansen commented Dec 15, 2017

Hi @addaleax I added many more versions of node to my CI build I I've seen some versions with inconsistent failures but one thing that seems to be consistent is that the first issue takes place in 8.3.0 I checked the change log and I have seem that 8.3.0 introduced a change that is directly related with this:

[357873d] - (SEMVER-MINOR) v8: fix stack overflow in recursive method (Ben Noordhuis) #14004

My guess is that somethign wen't wrong with that change but I know nothing about V8 and C++ so I can't help here :(

Maybe @bnoordhuis can help here?

@remojansen remojansen changed the title Node > 9.1.0 not throwing "Maximum call stack size exceeded"? Node >= 9.1.0 not throwing "Maximum call stack size exceeded"? Dec 15, 2017
@remojansen remojansen changed the title Node >= 9.1.0 not throwing "Maximum call stack size exceeded"? Node >= 9.1.0 not throwing "Maximum call stack size exceeded" Dec 15, 2017
remojansen added a commit to inversify/InversifyJS that referenced this issue Dec 15, 2017
@remojansen remojansen changed the title Node >= 9.1.0 not throwing "Maximum call stack size exceeded" Node >= 8.3.0 inconsistently not throwing "Maximum call stack size exceeded" Dec 15, 2017
@remojansen
Copy link
Author

remojansen commented Dec 15, 2017

I have tried to change my implementation.

I was using the following:

function factoryWrapper<T extends Function>(
    factoryType: FactoryType,
    serviceIdentifier: interfaces.ServiceIdentifier<any>,
    factory: T
) {
    return (...args: any[]) => {
        try {
            return factory(...args);
        } catch (error) {
            if (isStackOverflowExeption(error)) {
                throw new Error(
                    ERROR_MSGS.CIRCULAR_DEPENDENCY_IN_FACTORY(factoryType, serviceIdentifier)
                );
            } else {
                throw error;
            }
        }
    };
}

To wrap some factories:

public toAutoFactory<T2>(serviceIdentifier: interfaces.ServiceIdentifier<T2>): interfaces.BindingWhenOnSyntax<T> {
        this._binding.type = BindingTypeEnum.Factory;
        this._binding.factory = (context) => {
            const autofactory = () => context.container.get<T2>(serviceIdentifier);
            return factoryWrapper(
                "toAutoFactory",
                this._binding.serviceIdentifier.toString(),
                autofactory
            );
        };
        return new BindingWhenOnSyntax<T>(this._binding);
    }

public toProvider<T2>(provider: interfaces.ProviderCreator<T2>): interfaces.BindingWhenOnSyntax<T> {
        this._binding.type = BindingTypeEnum.Provider;
        this._binding.provider = factoryWrapper(
            "toProvider",
            this._binding.serviceIdentifier.toString(),
            provider
        );
        return new BindingWhenOnSyntax<T>(this._binding);
    }

Now I'm wrapping them in a different way:

const invokeFactory = (
    factoryType: FactoryType,
    serviceIdentifier: interfaces.ServiceIdentifier<any>,
    fn: () => any
) => {
    try {
        return fn();
    } catch (error) {
        if (isStackOverflowExeption(error)) {
            throw new Error(
                ERROR_MSGS.CIRCULAR_DEPENDENCY_IN_FACTORY(factoryType, serviceIdentifier.toString())
            );
        } else {
            throw error;
        }
    }
};
} else if (binding.type === BindingTypeEnum.Provider && binding.provider !== null) {
    result = invokeFactory(
        "toProvider",
        binding.serviceIdentifier,
        () => binding.provider ? binding.provider(request.parentContext) : undefined
    );
} else if (binding.type === BindingTypeEnum.Instance && binding.implementationType !== null) {
    result = resolveInstance(
        binding.implementationType,
        childRequests,
        _resolveRequest(requestScope)
    );
}

The only difference between the previous and the new implementations is that in the previous (the failing one) there is an extra closure return (...args: any[]) => { however the CI results are very different: https://travis-ci.org/inversify/InversifyJS/builds/316903

@bnoordhuis It happens in 8.0.0 so please forget about the 8.3.0 commit

@remojansen remojansen changed the title Node >= 8.3.0 inconsistently not throwing "Maximum call stack size exceeded" Node >= 8.0.0 inconsistently not throwing "Maximum call stack size exceeded" Dec 15, 2017
@remojansen
Copy link
Author

Thanks for your time and sorry but now I think it was a case of #14311 the only thing I can suggest since this is a known limitation is maybe try to come up with a way to warn the developers about this but I understand that it is not an easy one.

remojansen added a commit to inversify/InversifyJS that referenced this issue Dec 15, 2017
* Docs improvements

* Sollved incorrect circular dependencies errors

* Preparing release

* better naming for file and vars

* Fixed issue in node 9.2.0

* Disabled node latest until nodejs/node/issues/17684 is resolved

* Update .travis.yml

* Update .travis.yml

* Update .travis.yml

* Update .travis.yml

* Update .travis.yml

* Update .travis.yml

* Trying to fix stack issue

* Update appveyor.yml

* Trying to fix stack error

* WIP

* Debug helper

* WIP

* Now that the circular dependency issue is fixed there is no need for so many versions of node

* Removed console.log
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

2 participants