test(xsnap): size limits

[dckc]

just getting started on limits

the goal is:
fixes #2652

[dckc]

@warner this tests all the limits I can think of. Does it suffice to close #2652?

[warner]

If I read this right, we're checking that:

• medium numbers of property names work, large numbers fail with a not enough keys error
• using eval to parse large 1+1,1+1,... strings works until about 1MiB, then fails (with an indeterminate error)
• building large TypedArrays fails with an indeterminate error that cannot be caught

That sounds pretty good. I'd like to be confident that the engine is catching these problems and exiting with a panic (SIGABRT or SIGKILL), rather than segfaulting (SIGSEGV). Is there any way we can check that? Maybe look for a particular message text?

If we're demonstrating the uncatchability of OOM in the last test, should we also do a catch for the first two?

Is a 1MiB limit for eval large enough to load our contract bundles? That seems kind of small compared to the bundle sizes I've seen. If that's the sort of thing we add control over (maybe in an option to xsnap()), we might want to have a test that shows a string of size N fails to eval when the option is set below N, but succeeds when the option is increased.

[dckc]

If I read this right, we're checking that:

• medium numbers of property names work, large numbers fail with a not enough keys error

yes.

• using eval to parse large 1+1,1+1,... strings works until about 1MiB, then fails (with an indeterminate error)

the intent was at least 1MiB. I think on my machine it gets 3 or 4 iterations past that.

• building large TypedArrays fails with an indeterminate error that cannot be caught

yes.

That sounds pretty good. I'd like to be confident that the engine is catching these problems and exiting with a panic (SIGABRT or SIGKILL), rather than segfaulting (SIGSEGV). Is there any way we can check that? Maybe look for a particular message text?

I believe the xsnap API returns the actual signal. I know it at least knows the signal internally, so I can surface it for this purpose.

If we're demonstrating the uncatchability of OOM in the last test, should we also do a catch for the first two?

I suppose so.

Is a 1MiB limit for eval large enough to load our contract bundles?

No ideer. I picked it arbitrarily.

That seems kind of small compared to the bundle sizes I've seen. If that's the sort of thing we add control over (maybe in an option to xsnap()), we might want to have a test that shows a string of size N fails to eval when the option is set below N, but succeeds when the option is increased.

I asked about an xsnap option in #2652 (comment) but feedback from @kriskowal suggested it's not time yet.

@kriskowal is it time now?

[dckc]

• using eval to parse large 1+1,1+1,... strings works until about 1MiB, then fails (with an indeterminate error)
  Is a 1MiB limit for eval large enough to load our contract bundles? That seems kind of small compared to the bundle sizes I've seen. If that's the sort of thing we add control over (maybe in an option to xsnap()), we might want to have a test that shows a string of size N fails to eval when the option is set below N, but succeeds when the option is increased.

fixed in 165ac31 . turns out XS is pretty well-behaved when parsing long strings (it was exiting with "too much computation" in previous tests). What it doesn't like is long string literals. But even then eval just throws a buffer overflow exception. Anyway, now we have a -s <size> (in kB) cli option and a parserBufferSize option in the js API.

• building large TypedArrays fails with an indeterminate error that cannot be caught
  That sounds pretty good. I'd like to be confident that the engine is catching these problems and exiting with a panic (SIGABRT or SIGKILL), rather than segfaulting (SIGSEGV). Is there any way we can check that? Maybe look for a particular message text?

turns out xsnap was catching these errors but exiting with a 0 status code.

In 3f5b9f0 we expose the exit code (and test for it).

If we're demonstrating the uncatchability of OOM in the last test, should we also do a catch for the first two?

done in 445d327

[warner]

Maybe let's make that an unsigned int? Since atoi accepts -1234, it might be an extra safety belt.

[dckc]

you want it to underflow and wrap around? I don't understand why.

[warner]

Good, but if this managed to catch the first few times, and then stopped catching it, we'd not be able to distinguish that failure mode from the intended one. Would it be possible to have the catch clause raise an exception that would cause the worker to exit with something measurably different (something other than exited with code 6) ?

If that's too much hassle, just having it print a "wait a minute I shouldn't be printed" and manually eyeball it once should be sufficient.

[dckc]

I don't understand your point. The try ... catch is outside the for loop, so it catches either 0 or 1 times. If it catches, the vat.evaluate won't reject, so it'll fail t.throwsAsync.

In any case, I put a for-ever loop in the catch block to exit with "too much computation" in 0b4501f .

[warner]

Minor changes suggested, but if they're too intrusive then feel free land without them.

[kriskowal]

This no longer validates the xsnap name option.

Might it make sense for the xsnap utility to convert machine error codes into humane error messages? There’s Node.js precedent for having an error.code property for automated recovery logic.

[dckc]

This no longer validates the xsnap name option.

The test label said it was about infinite loops, not the name option. But OK, this is a regression, so I un-did it in b349045 .

Might it make sense for the xsnap utility to convert machine error codes into humane error messages? There’s Node.js precedent for having an error.code property for automated recovery logic.

Perhaps... but I prefer to postpone it indefinitely as a separate issue: #2749 .