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.

Sign up for GitHub

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

Jump to bottom

Provide more context on infinites recursion #230

Closed
SimenB opened this issue Feb 5, 2019 · 9 comments
Closed

Provide more context on infinites recursion #230

SimenB opened this issue Feb 5, 2019 · 9 comments
Labels
difficulty: medium help wanted pinned

Comments

@SimenB
Copy link
Member

SimenB commented Feb 5, 2019

We understand you have a problem and are in a hurry, but please provide us with some info to make it much more likely for your issue to be understood, worked on and resolved quickly.

  • Lolex version : 3.0
  • Environment : N/A
  • Example URL : N/A
  • Other libraries you are using: N/A

What did you expect to happen?

See jestjs/jest#2893 for details.

Tl;dr: Right now Lolex just throws an error if it thinks we're in an infinite loop of timers (timers scheduling new timers). This comes from here: https://github.com/sinonjs/lolex/blob/29b4ecc8bd186584ab4139341f3128a4349463ba/lolex.js#L210

It would be nice if the message thrown included a stack trace back to the code actually scheduling timers

What actually happens

An error without context

How to reproduce
https://runkit.com/embed/jqyax6pkc5bq
@SimenB SimenB mentioned this issue Feb 5, 2019
Closed
@benjamingr
Copy link
Member

benjamingr commented Feb 5, 2019 •
edited
Loading

We can add the stack to clocks when they are captured and show it on the clock when this is thrown - though this will be really expensive CPU wise (taking a ton of stack traces).

I'm open to other ideas.

@SimenB
Copy link
Member Author

SimenB commented Feb 5, 2019

What about, after reaching loopLimit, you schedule 10 more or so, and capture just their stacks, then throw? So in the normal case there is no change, but you loop a few extra times (maybe even loopLimit - 10 to not really affect anything observable) in the case where things are gonna throw anyways?

@benjamingr
Copy link
Member

So:

  • If I'm inside lolex and I'm at loopLimit - 10
  • Then start capturing stack traces when setTimeout/setInterval etc are called
  • If the loop limit is reached before another timer is hit, fail without a stack
  • Otherwise fail with the stack any of those timers.

Sounds useful/good to me and like it should solve the 95% case.

@SimenB
Copy link
Member Author

SimenB commented Feb 5, 2019

Yeah, that sounds reasonable to me! Potentially start with just a single stack instead of 10, not sure? Changing that threshold is simple once the logic is in place though, so not something we need to spend time on deciding now

@SimenB
Copy link
Member Author

SimenB commented Feb 5, 2019

  • If the loop limit is reached before another timer is hit, fail without a stack

Wait, no - not this. If loopLimit is 100, we don't want to fail if we get to 95 timers before bailing

@benjamingr
Copy link
Member

Yeah, that

@stale
Copy link

stale bot commented Apr 6, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 6, 2019
@benjamingr benjamingr removed the stale label Apr 7, 2019
@stale
Copy link

stale bot commented Jun 6, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 6, 2019
@SimenB SimenB added the pinned label Jun 6, 2019
@stale stale bot removed the stale label Jun 6, 2019
alistairjcbrown added a commit to alistairjcbrown/fake-timers that referenced this issue May 17, 2020
@alistairjcbrown
Add stack trace to code recursively scheduling timers
9a92f55 
Use stack information for jobs and timers to provide more context when
an infinite loop error is thrown.

Fixes sinonjs#230
@alistairjcbrown alistairjcbrown mentioned this issue May 17, 2020
Merged
@alistairjcbrown
Copy link
Contributor

Put up PR #325 to add stack trace output for the next excess job to be run - let me know if you think that's the right direction for solving this issue

alistairjcbrown added a commit to alistairjcbrown/fake-timers that referenced this issue May 17, 2020
@alistairjcbrown
Add stack trace to code recursively scheduling timers
290426e 
Use stack information for jobs and timers to provide more context when
an infinite loop error is thrown.

Fixes sinonjs#230
alistairjcbrown added a commit to alistairjcbrown/fake-timers that referenced this issue May 17, 2020
@alistairjcbrown
Add stack trace to code recursively scheduling timers
8e38995 
Use stack information for jobs and timers to provide more context when
an infinite loop error is thrown.

Fixes sinonjs#230
itayperry pushed a commit to itayperry/fake-timers that referenced this issue May 1, 2021
@alistairjcbrown @itayperry
Add stack trace to code recursively scheduling timers
a4fb0b4 
Use stack information for jobs and timers to provide more context when
an infinite loop error is thrown.

Fixes sinonjs#230
itayperry pushed a commit to itayperry/fake-timers that referenced this issue Jun 4, 2021
@alistairjcbrown @itayperry
Add stack trace to code recursively scheduling timers
ffe4078 
Use stack information for jobs and timers to provide more context when
an infinite loop error is thrown.

Fixes sinonjs#230

Add flag for approaching infinite loop

Use a flag when approaching infinite loop detection which will enable
an error object to be attached to the job, which can then be used for
generating the stack output.

Update for PR feedback

- Safely update stack property using `defineProperty`, wrapped in try/catch
- Tests for more clock functions to confirm stack trace provided
- Stack trace slice automatically calculated based on clock function position

Remove unnecessary durations and use globals

- Use global setInterval and setImmediate to match setTimeout use
- Remove unnecessary duration parameter from setImmediate and setImmediate

add use of prettier

Firefox and Chrome tests pass after RegExp fix

Chrome shows stack messages differently, and uses the words: 'at' and 'Object', Firefox does not - that has created two problems: first, the stack trace messages that are shown to the user are a bit different (Firefox always has at least one more line at the start of the stack list - usually internal functions that are not supposed to appear). Second, the RegExp tried to match parts of the strings that only exist in Chrome, failing the Firefox tests)

prettier used
fatso83 pushed a commit that referenced this issue Jun 8, 2021
@itayperry @alistairjcbrown
Retry - Add stack trace to code recursively scheduling timers #325 (#375
54c2e8d 
)

* Add stack trace to code recursively scheduling timers

Use stack information for jobs and timers to provide more context when
an infinite loop error is thrown.

Fixes #230

Add flag for approaching infinite loop

Use a flag when approaching infinite loop detection which will enable
an error object to be attached to the job, which can then be used for
generating the stack output.

Update for PR feedback

- Safely update stack property using `defineProperty`, wrapped in try/catch
- Tests for more clock functions to confirm stack trace provided
- Stack trace slice automatically calculated based on clock function position

Remove unnecessary durations and use globals

- Use global setInterval and setImmediate to match setTimeout use
- Remove unnecessary duration parameter from setImmediate and setImmediate

add use of prettier

Firefox and Chrome tests pass after RegExp fix

Chrome shows stack messages differently, and uses the words: 'at' and 'Object', Firefox does not - that has created two problems: first, the stack trace messages that are shown to the user are a bit different (Firefox always has at least one more line at the start of the stack list - usually internal functions that are not supposed to appear). Second, the RegExp tried to match parts of the strings that only exist in Chrome, failing the Firefox tests)

prettier used

* small RegExp fix for node env + linting

* improving test-coverage statistics

* at attempt to ignore certain condition in nyc coverage

Co-authored-by: Alistair Brown <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
difficulty: medium help wanted pinned
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@fatso83 @alistairjcbrown @benjamingr @SimenB