fix: avoid call stack recursion with large error (fix: #3060)

[nathanmmiller]

Fixes #3060

Full details in original issue. I have tested this with the same code used therein - I did not include that code in here because it relies on too many external libraries to make such a massive error output.

[nathanmmiller]

@sheremet-va are you the right person to review this? I see you tagged my original issue. Am keen to close this out as it's preventing me from moving to vitest, which I'd love to do.

[sheremet-va]

@sheremet-va are you the right person to review this? I see you tagged my original issue. Am keen to close this out as it's preventing me from moving to vitest, which I'd love to do.

I don't really understand how this fixes it? Why do we need an if? Why can't we always do what is done for 5000 characters?

[nathanmmiller]

@sheremet-va are you the right person to review this? I see you tagged my original issue. Am keen to close this out as it's preventing me from moving to vitest, which I'd love to do.

I don't really understand how this fixes it? Why do we need an if? Why can't we always do what is done for 5000 characters?

You mean just replace the current implementation with my change for <5000? I'm happy to do so if you'd prefer that. I assumed (maybe wrongly) there was a reason why the code c.red() the entire error message instead of just the FAIL part.

It seems to me that logger.error(${c.red(c.bold(errorName))}: ${error.message}) is the better code for all instances instead of an if, but I didn't want to just get rid of the existing implementation if there was a good reason for it. Happy to do so though! Lemme know and I can re-raise a PR with that change.

[sheremet-va]

You mean just replace the current implementation with my change for <5000? I'm happy to do so if you'd prefer that. I assumed (maybe wrongly) there was a reason why the code c.red() the entire error message instead of just the FAIL part.

Ah, now I see 🤦🏻

I don't know then 🤔 Maybe it's possible to fix the recursion itself? Maybe use tail recursion 🤔

[nathanmmiller]

Ah, now I see 🤦🏻

I don't know then 🤔 Maybe it's possible to fix the recursion itself? Maybe use tail recursion 🤔

I don't think fixing the underlying recursion will be possible without a change to picocolors, not vitest. The real issue here is that RTL also colors its output - so we end up running a color-fied output through c.red() which then fills the stack with a zillion calls. It seems to only break at >7000 characters (the default output size for RTL actually, so with the default you're usually fine) but I chose 5000 as a safe buffer.

My assumption was:
if output.length > 5000, it's likely that the output comes from something (like RTL) that already has done formatting/coloring, and we shouldn't .red() it anyway.

I understand your hesitance for the if/else - I think if you're not good with that approach I'd rather submit to just not c.red() the error itself at all - and only do the FAIL part - it seems like a "nice to have" (red error details) is not worth a stack overflow (ability to actually see full output from RTL, a very common use case).

Thoughts?

[nathanmmiller]

@sheremet-va any tweaks you want me to make? I really do want to use vitest in our project but this issue is a blocker. Thanks!

[sheremet-va]

I don't think fixing the underlying recursion will be possible without a change to picocolors, not vitest

We have a forked version in the monorepo in utils/src/colors.ts file. It's used mostly for runtime, not for Node.js logic, but I guess we can set it up here. The only problem is that we need to set it up somewhere, and I'm not sure where.

[nathanmmiller]

I don't think fixing the underlying recursion will be possible without a change to picocolors, not vitest

We have a forked version in the monorepo in utils/src/colors.ts file. It's used mostly for runtime, not for Node.js logic, but I guess we can set it up here. The only problem is that we need to set it up somewhere, and I'm not sure where.

Is that something I should wait for or is there an action item for me here on this PR?

[nathanmmiller]

How about I raise a PR for just the code path with red() only on FAIL and not on the rest of the message (so no if/else) so that we can unblock this issue, and then if there are future changes to be made, we can raise a new issue for them? Would that work @sheremet-va ?

[sheremet-va]

How about I raise a PR for just the code path with red() only on FAIL and not on the rest of the message (so no if/else) so that we can unblock this issue, and then if there are future changes to be made, we can raise a new issue for them? Would that work @sheremet-va ?

I made a library tinyrainbow where we can fix the call stack issue. The idea is to replace picocolors in Vitest repo in Node and in the browser environments.

[nathanmmiller]

So to be clear @sheremet-va - there's nothing for me to do here and at some point this issue will be closed by a different PR? If so, I shall wait and then migrate our code base once that is done.

[sheremet-va]

So to be clear @sheremet-va - there's nothing for me to do here and at some point this issue will be closed by a different PR? If so, I shall wait and then migrate our code base once that is done.

Tinyrainbow still has the bug. Feel free to fix it there

[nathanmmiller]

@sheremet-va I've played around with tinyrainbow a little and I don't think it's going to be realistically solvable there either - the fundamental problem is that there is - with large pre-colored inputs - a stack overflow no matter what.

Tail recursion does not solve this issue, it just will kick the can down the road (so an even larger pre-colored input) still will break it) and most js interpreters can't even optimize for tail recursion as far as I can tell.

I don't think this is the solution we want, and I think that with others now facing the same issue outside of just RTL, it would be a good idea to use my original PR: a simple test to say "probably color this, but if the input is very large assume it's already colored and don't bother doing so." This would unblock our adoption and solve the issue for other people as well.

I really want to onboard to vitest, but this is a critical blocker for us. I cannot use your library without it being solved.

[sheremet-va]

@sheremet-va I've played around with tinyrainbow a little and I don't think it's going to be realistically solvable there either - the fundamental problem is that there is - with large pre-colored inputs - a stack overflow no matter what.

Tail recursion does not solve this issue, it just will kick the can down the road (so an even larger pre-colored input) still will break it) and most js interpreters can't even optimize for tail recursion as far as I can tell.

I don't think this is the solution we want, and I think that with others now facing the same issue outside of just RTL, it would be a good idea to use my original PR: a simple test to say "probably color this, but if the input is very large assume it's already colored and don't bother doing so." This would unblock our adoption and solve the issue for other people as well.

I really want to onboard to vitest, but this is a critical blocker for us. I cannot use your library without it being solved.

I see. Let's just guard it like that for the time being then.

[nathanmmiller]

@sheremet-va sorry to bother - when should we anticipate a merge/release? Thanks in advance!

[sheremet-va]

@sheremet-va sorry to bother - when should we anticipate a merge/release? Thanks in advance!

0.32.1 should be this week.