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

Failing test case for recursive types #940

Merged
merged 3 commits into from
Jun 10, 2017

Conversation

DullReferenceException
Copy link
Contributor

XML generation in a service with a recursively-defined type is causing a stack overflow. There appears to be infinite recursion going on. Here's the repeating stack trace fragment:

      at WSDL.findChildSchemaObject (/Users/jpage/Code/node-soap/lib/wsdl.js:1978:19)
      at WSDL.findChildSchemaObject (/Users/jpage/Code/node-soap/lib/wsdl.js:1984:20)
      at WSDL.findChildSchemaObject (/Users/jpage/Code/node-soap/lib/wsdl.js:1984:20)
      at WSDL.findChildSchemaObject (/Users/jpage/Code/node-soap/lib/wsdl.js:1978:19)
      at WSDL.findChildSchemaObject (/Users/jpage/Code/node-soap/lib/wsdl.js:1984:20)
      at WSDL.findChildSchemaObject (/Users/jpage/Code/node-soap/lib/wsdl.js:1984:20)

This boiled-down version of our code in the new test case reproduces it.

Note that versions before 0.15.0 did not have this bug.

Created failing test case for recursive types with the hopes that we can
figure out how to fix it.
@coveralls
Copy link

coveralls commented Jun 9, 2017

Coverage Status

Coverage remained the same at 93.249% when pulling 1d2ccd8 on DullReferenceException:recursive-bug-repro into 73fbdbf on vpulim:master.

@jsdevel
Copy link
Collaborator

jsdevel commented Jun 9, 2017

Thanks @DullReferenceException ! Hopefully someone can pick this up and submit a PR. Any ideas @herom on what may have introduced this?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.249% when pulling 7513c4b on DullReferenceException:recursive-bug-repro into 73fbdbf on vpulim:master.

1 similar comment
@coveralls
Copy link

coveralls commented Jun 9, 2017

Coverage Status

Coverage remained the same at 93.249% when pulling 7513c4b on DullReferenceException:recursive-bug-repro into 73fbdbf on vpulim:master.

@coveralls
Copy link

coveralls commented Jun 9, 2017

Coverage Status

Coverage remained the same at 93.249% when pulling 082ef27 on DullReferenceException:recursive-bug-repro into 73fbdbf on vpulim:master.

@coveralls
Copy link

coveralls commented Jun 10, 2017

Coverage Status

Coverage increased (+0.02%) to 93.266% when pulling 5a905b6 on DullReferenceException:recursive-bug-repro into 73fbdbf on vpulim:master.

@DullReferenceException
Copy link
Contributor Author

Aw yeah, found a fix which is basically just maintaining a recursion backtrace. Does this fix look ok?

@jsdevel jsdevel merged commit 85c8409 into vpulim:master Jun 10, 2017
@jsdevel
Copy link
Collaborator

jsdevel commented Jun 10, 2017

Amazing @DullReferenceException ! Thanks!

@DullReferenceException DullReferenceException deleted the recursive-bug-repro branch June 10, 2017 02:01
@DullReferenceException
Copy link
Contributor Author

Thank you for the quick merge!

@jsdevel
Copy link
Collaborator

jsdevel commented Jun 10, 2017

Np!

@DullReferenceException
Copy link
Contributor Author

@jsdevel: any estimate on when this will be published?

@jsdevel
Copy link
Collaborator

jsdevel commented Jun 12, 2017

Published as v0.19.2 @DullReferenceException

// We've recursed back to ourselves; break.
return null;
} else {
backtrace = backtrace.concat([parameterTypeObj]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to understand it better: what's the purpose of using concat() and creating a new Array instead of just push()ing ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we mutate the array (in the case when it's passed in), then after this function returns, the array will erroneously carry the new value to the next iteration, making it no-longer reflect the backtrace.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always eager to learn new things but I'm afraid I not able to follow you here 😸

Would it be possible for you to create a jsbin or gist or something like that in order for me to get the difference? Maybe it's because it's already late in the evening (for me) 🇦🇹 I would be very thankful for this 👍 💯

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Let's say we're traversing this schema:

foo
  bar
    foo
    baz
  blah
zub

If we used .push(), here's what the array would look like as we do the depth-first traversal:

[foo]
[foo, bar]
// (here we encounter foo again, so we skip)
[foo, bar, baz]
[foo, bar, baz, blah] // Incorrect; should be [foo, blah]
[foo, bar, baz, blah, zub] // Incorrect; should be [zub]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks a ton @DullReferenceException - it seems it was really too late for me 😄 - totally clear now 👍

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