Update node version 14.15.5 -> 15.14.0

[sbc100]

This is the current v15 release and updating to v15 or above will allow
is to disable and possibly completely remove the
NODEJS_CATCH_REJECTION setting in emscripten. This setting is a
workaround for the fact the older node versions will return exit with 0
on unhandled promise rejection.

The downside to updating the version used by emsdk and therefore the
version against which we do all our emscripten testing is that we
potentially loose some coverage of compatability with older versions.
In practice I don't think we have seen a node compat issue in many
years. The limits on the JS output produced by emscripten are all
related to targeting older browsers which tend to be a lot older than
any of the node versions we want to target.

[kripken]

I think I don't understand the motivation here yet. The problem with NODEJS_CATCH_REJECTION is not in our usage of node.js during compilation, but when our users run node to run the compiled code. It's probably most common to run the code on a different machine, one without an emsdk build, and to just use the local node. That is, updating node in emscripten will not allow us to move forward here, I don't think?

If that is right, then it seems risky to make this change. We should test the node that most users will be running. I think the LTS version is the best guess at that (as it is what the node.js site offers as the default download).

However, if we think the NODEJS_CATCH_REJECTION issue is urgent to fix, we can perhaps remove it but add in a version check for node at runtime in assertion builds. That is, the reason we need NODEJS_CATCH_REJECTION is to not ignore errors (which could be very bad for people's test suites). If we show some kind of error in assertions builds, that seems like enough to avoid that risk. And if so, we might as well update node in emscripten if it simplifies our testing, etc. - is that the idea here?

[sbc100]

I think I don't understand the motivation here yet. The problem with NODEJS_CATCH_REJECTION is not in our usage of node.js during compilation, but when our users run node to run the compiled code. It's probably most common to run the code on a different machine, one without an emsdk build, and to just use the local node. That is, updating node in emscripten will not allow us to move forward here, I don't think?

If that is right, then it seems risky to make this change. We should test the node that most users will be running. I think the LTS version is the best guess at that (as it is what the node.js site offers as the default download).

However, if we think the NODEJS_CATCH_REJECTION issue is urgent to fix, we can perhaps remove it but add in a version check for node at runtime in assertion builds. That is, the reason we need NODEJS_CATCH_REJECTION is to not ignore errors (which could be very bad for people's test suites). If we show some kind of error in assertions builds, that seems like enough to avoid that risk. And if so, we might as well update node in emscripten if it simplifies our testing, etc. - is that the idea here?

My understanding is that NODEJS_CATCH_REJECTION was added mostly to make out unit test fail reliably.

Folks using emscripten + nodejs in wild will most likely be using the emscripten-built module with other surrounding JS code and they will want to promise rejections in whatever way they see fit and probably don't want emscripten messing with this globally across their entire program.

If I'm wrong, we can just leave NODEJS_CATCH_REJECTION in there as an option, but I'd us to be able to stop depending on it in our test suite at least.

[kripken]

Is the benefit worth the cost? I think running the most common node is safer for us, to match our users. How do you see the benefit of this change, maybe that's what I don't get? (does it allow us to remove any code?)

[sbc100]

Is the benefit worth the cost? I think running the most common node is safer for us, to match our users. How do you see the benefit of this change, maybe that's what I don't get? (does it allow us to remove any code?)

1. We remove this line of code:

process['on']('unhandledRejection', abort);

This is an example of a place where emscripten is messing with global, process-wide, state in way that it really doesn't need to. Doing this kind of thing is really something that belongs in the overall node application, not just in the emscripten module.

2. We get better error message

I have not proved this but I think the nodes internal rejection handler will give better error messages that show more clearly the source of the issue rather then going via abort.

Personally I often disable both NODEJS_CATCH_EXIT and NODEJS_CATCH_REJECTION when debugging node issues because they seem to make the reporting of the actual error less useful. It could be that its NODEJS_CATCH_EXIT that is more to blame.

[sbc100]

Regardless of what we do with NODEJS_CATCH_REJECTION we should be able to land this change as is. Assuming there are not downsides to simply upgrading node we can then experiment with disabling NODEJS_CATCH_REJECTION maybe just in the test suite if it does result in better error reporting. We can have the debate over in the emscripten repo after this change lands?

[kripken]

What do you mean by "we can remove process['on']('unhandledRejection', abort);" - ?

My argument above is that we can't remove it for our users. We can remove just for our own test suite, but our users would still need it (since we don't control the node they use in their test suites - assuming it is the default node download is the most reasonable guess we can make), so we couldn't remove it from our codebase here (only disable it for our own uses).

[sbc100]

What do you mean by "we can remove process['on']('unhandledRejection', abort);" - ?

My argument above is that we can't remove it for our users. We can remove just for our own test suite, but our users would still need it (since we don't control the node they use in their test suites - assuming it is the default node download is the most reasonable guess we can make), so we couldn't remove it from our codebase here (only disable it for our own uses).

I think we might be able to remove it by default and have folks opt into it.

I think its only useful to a certain subset of users who meed two conditions:

a. targeting older versions of node
b. are using emscripten-generated JS as their entire program.

Users who are embedding an emscripten module inside a wider application probably don't want this global handler to be controlled by the emscripten module.

I would argue that (b) is not how real world users use emscripten under node... even though that is how most our test suite works.

[sbc100]

I think we can land this upgrade either way since it offers us more flexibility going forward.

[kripken]

I think we might be able to remove it by default and have folks opt into it. I think its only useful to a certain subset of users who meed two conditions:

a. targeting older versions of node

My concern is that it isn't an older version - it's the current default download that node.js gives people. That's probably most of our users, I think!

And the risk is that if they do not know to flip the flag on, then their test suites will silently hide errors. That kind of thing is quite dangerous I think.

b. are using emscripten-generated JS as their entire program.

This is a fair point that I had not considered before. Yes, perhaps most people's test suites are more modular. But, I am not sure - how can we tell?

I think we can land this upgrade either way since it offers us more flexibility going forward.

It does give us flexibility, but it means we are not testing the default node download any more. But testing the most likely version our users use is the best thing, isn't it? We'd need a strong reason to do otherwise I think, and I don't see that.

[sbc100]

b. are using emscripten-generated JS as their entire program.

This is a fair point that I had not considered before. Yes, perhaps most people's test suites are more modular. But, I am not sure - how can we tell?

I wasn't really thinking about test suites in particular. I was thinking about folks using emscripten module in production node environments.

I think we can land this upgrade either way since it offers us more flexibility going forward.

It does give us flexibility, but it means we are not testing the default node download any more. But testing the most likely version our users use is the best thing, isn't it? We'd need a strong reason to do otherwise I think, and I don't see that.

Yes and no. Our node testing is really a proxy for browser testing, isn't it? Aren't we assuming that deployments will be massively skewed towards the browser deployments? If we accept that premise then most of our unit tests should really run under and environment that matches the current version of v8 that is running in most user's browsers. Doing most of our testing on an old version of v8 (such as the one in node's LTS) could be seen as a bad thing. I'm not saying this argument wins (because we do have some production node users for sure), but there are certainly two conflicting things that our node unit tests are trying to do here.

Perhaps we should have a MIN_NODE_VERSION so that older node versions can be still be targeted? Of course we would have the same problem we have with MIN_CHROME_VERSION in that we don't do any actual testing on any version except the current one.

[dschuff]

I think it's still a fairly common use case that people build unit tests and run them in node in addition to whatever browser testing they do, for all the same reasons that we do it.
For our purposes, we will often want as new a version of node as we can get, because we want to test bleeding-edge features. I'd guess that our users will want older and/or LTS versions of node, or won't care so much, as long as it supports the browser features they care about (which are probably different for different users).

[sbc100]

I think it's still a fairly common use case that people build unit tests and run them in node in addition to whatever browser testing they do, for all the same reasons that we do it.
For our purposes, we will often want as new a version of node as we can get, because we want to test bleeding-edge features. I'd guess that our users will want older and/or LTS versions of node, or won't care so much, as long as it supports the browser features they care about (which are probably different for different users).

I think if users are running unit tests for code that will ultimately run in the browser in production they will probably have the same requirements we do, and will most likely use the version of node that ships with emsdk (will will be happy to use that versions if its better/easier).

I think the risk here is that folks who are running code under node in production.

One easy way to mitigate all the risk here I think would be to set MIN_NODE_VERSION = EMSDK_NODE_VERSION and then include a check in debug builds:

assert(NODE_VERSION >= MIN_NODE_VERSION, please build with MIN_NODE_VERSION = XXX to run on older node versions).

[kripken]

Our node testing is really a proxy for browser testing, isn't it?

Good point, yes, the last comments really clarify the issues here for me, starting from here.

We can maybe just ask on the mailing list to get feedback from people testing with node. My guess, however, differs from yours @sbc100 - when setting up testing on say ammo.js and other libraries, the easiest thing is to use the existing node.js on the system, not the emsdk's. That may even be older than the default node.js download from their website - but I don't think we need to be more careful than the website.

An error in debug builds that checks for the node version may help here. But if many users end up needing to flip flags to get things to work in their current node, that's a big downside I think.

Another approach might be to investigate in more detail the errors that we worry about being silenced by older node versions. If it's just errors during compiling the wasm, maybe we can add a special check there, for example.

Yet another possibility is to use sync node startup by default, assuming that avoids the silencing issue (which I suspect but am not sure of). That has other downsides in terms of testing, but might be worth considering.

[sbc100]

For the record this is why we can't current update to node TLS: nodejs/node#43246

Our bots run ubuntu bionic which is not supported by node 18 and above :(

[Shaikh-Ubaid]

With the latest emsdk using node 15.14.0, we are unfortunately facing a failure at our CI https://github.com/lfortran/lfortran/actions/runs/4673006168/jobs/8275761272.

It seems the command that fails is node src/lfortran/tests/test_lfortran.js. The Github CI halts with Error: The operation was canceled.. Since, there is not much description about why the operation got cancelled, it is being tricky to debug. Any idea why it could be failing? Using the older node 14.18.2 at the CI seems to work Shaikh-Ubaid/lfortran#13.

I also tested the command node src/lfortran/tests/test_lfortran.js locally on my system using node 18.13.0 and it seems to work (exiting with code 0). Thus, I guess there is no occurrence of UnhandledPromiseRejection.

[sbc100]

Interesting. I think we should try to figure out exactly what is going on here. Could you perhaps attach the failing JS file (along with any dependencies such as the wasm binary)?

As a workaround could you perhaps use the system version node to execute your test? With luck it could be a version of node the doesn't have this problem. What OS and version is your test running on?

[Shaikh-Ubaid]

Please find attached the JS file and wasm binary (It seems GitHub does not support attaching wasm binaries, hence attaching as zip).
test_lfortran.zip

As a workaround could you perhaps use the system version node to execute your test?

Do you mean to execute it locally on my system? On my system, I have node 18.13.0 currently installed and the test_lfortran.js seems to run fine.

(base) ci_fix_build_to_wasm$ node -v
v18.13.0
(base) ci_fix_build_to_wasm$ node test_lfortran.js
[doctest] doctest version is "2.4.8"
[doctest] run with "--help" for options
===============================================================================
[doctest] test cases:  49 |  49 passed | 0 failed | 0 skipped
[doctest] assertions: 456 | 456 passed | 0 failed |
[doctest] Status: SUCCESS!
(base) ci_fix_build_to_wasm$ echo $?
0
(base) ci_fix_build_to_wasm$ 

With luck it could be a version of node the doesn't have this problem.

The bug seems to occur only at the GitHub CI. For example, https://github.com/Shaikh-Ubaid/lfortran/actions/runs/4667899568/jobs/8264264545 uses node 18.15.0 and fails with the same Error: The operation was canceled.

What OS and version is your test running on?

The test is running on ubuntu-22.04.

[sbc100]

So you are unable to reproduce this locally? No matter what version of node you use?

[sbc100]

So the lines that start with the green [doctest] strings are coming from the compiled emscripten code? That makes me think perhaps the line that starts with the red Error: line is also coming from the generated code. Does that sounds likely? Any idea where it might be coming from?

[Shaikh-Ubaid]

So you are unable to reproduce this locally? No matter what version of node you use?

Yes. Locally, I tested with node 14.18.2 (installed by emsdk and present inside the emsdk directory) and 18.13.0 (system wide installed, present in .nvm/versions/node/v18.13.0/bin/node. They both seem to work.

So the lines that start with the green [doctest] strings are coming from the compiled emscripten code?

Yes, they are coming from the wasm binary.

That makes me think perhaps the line that starts with the red Error: line is also coming from the generated code. Does that sounds likely? Any idea where it might be coming from?

It seems to me the red Error line is being printed by GitHub CI runner. For example, on searching on google, I found https://github.com/credativ/vali/actions/runs/4467520060/jobs/7847403788 which prints the same error.

[kripken]

@Shaikh-Ubaid Can you test with the new node version that this installs, 15.14.0? Maybe that one fails on your system somehow, even though 14.18.2 and 18.13.0 work.

[Shaikh-Ubaid]

Please find executions using different node versions. (the second one from top is node 15.14.0).

(lf) lfortran$ node -v
v14.18.2
(lf) lfortran$ which node
/home/ubaid/ext/emsdk/node/14.18.2_64bit/bin/node
(lf) lfortran$ node src/lfortran/tests/test_lfortran.js
[doctest] doctest version is "2.4.8"
[doctest] run with "--help" for options
===============================================================================
[doctest] test cases:  49 |  49 passed | 0 failed | 0 skipped
[doctest] assertions: 456 | 456 passed | 0 failed |
[doctest] Status: SUCCESS!
(lf) lfortran$ echo $?
0
(lf) lfortran$ 


(base) lfortran$ node -v
v15.14.0
(base) lfortran$ which node
/home/ubaid/.nvm/versions/node/v15.14.0/bin/node
(base) lfortran$ node src/lfortran/tests/test_lfortran.js
[doctest] doctest version is "2.4.8"
[doctest] run with "--help" for options
===============================================================================
[doctest] test cases:  49 |  49 passed | 0 failed | 0 skipped
[doctest] assertions: 456 | 456 passed | 0 failed |
[doctest] Status: SUCCESS!
(base) lfortran$ echo $?
0
(base) lfortran$ 


(base) lfortran$ node -v
v18.12.1
(base) lfortran$ which node
/home/ubaid/.nvm/versions/node/v18.12.1/bin/node
(base) lfortran$ node src/lfortran/tests/test_lfortran.js
[doctest] doctest version is "2.4.8"
[doctest] run with "--help" for options
===============================================================================
[doctest] test cases:  49 |  49 passed | 0 failed | 0 skipped
[doctest] assertions: 456 | 456 passed | 0 failed |
[doctest] Status: SUCCESS!
(base) lfortran$ echo $?
0
(base) lfortran$ 


(base) lfortran$ node -v
v18.13.0
(base) lfortran$ which node
/home/ubaid/.nvm/versions/node/v18.13.0/bin/node
(base) lfortran$ node src/lfortran/tests/test_lfortran.js
[doctest] doctest version is "2.4.8"
[doctest] run with "--help" for options
===============================================================================
[doctest] test cases:  49 |  49 passed | 0 failed | 0 skipped
[doctest] assertions: 456 | 456 passed | 0 failed |
[doctest] Status: SUCCESS!
(base) lfortran$ echo $?
0
(base) lfortran$ 


(base) lfortran$ node -v
v18.15.0
(base) lfortran$ which node
/home/ubaid/.nvm/versions/node/v18.15.0/bin/node
(base) lfortran$ node src/lfortran/tests/test_lfortran.js
[doctest] doctest version is "2.4.8"
[doctest] run with "--help" for options
===============================================================================
[doctest] test cases:  49 |  49 passed | 0 failed | 0 skipped
[doctest] assertions: 456 | 456 passed | 0 failed |
[doctest] Status: SUCCESS!
(base) lfortran$ echo $?
0
(base) lfortran$ 

They all seem to work fine on my system locally.

[sbc100]

Well thats rather annoying isn't it.. I guess one way to move forward would be to add some more debugging information to see where the version on github might be crashing. Can you try building with -sRUNTIME_DEBUG and -sASSERTIONS=2?

I also wonder if there are any node verbosity flags you could turn on? How about --trace-uncaught for example.

[Shaikh-Ubaid]

Added debugging flags https://github.com/Shaikh-Ubaid/lfortran/actions/runs/4685550543/jobs/8302754966?pr=14. It seems to exit fine (with exit code 0).

[sbc100]

From googling it looks like this message occurs when the github action times out, but I don't see any other evendence of this. It seems to happen always right after the node run.

Can you trying this: Create a run.sh script that runs the project under node and then does echo $? to print the node exit status?

[sbc100]

From the detailed log:

2023-04-13T04:54:44.3377380Z ##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.
2023-04-13T04:54:44.9313218Z ##[error]The operation was canceled.

Seems odd that the running would received a shutdown signal add the same place each time. Maybe worth filing a issue with github actions?

[Shaikh-Ubaid]

Can you trying this: Create a run.sh script that runs the project under node and then does echo $? to print the node exit status?

Sure, added script in CI: Create a debug_run.sh script. Still the same https://github.com/Shaikh-Ubaid/lfortran/actions/runs/4692825473/jobs/8319029416?pr=14.

[sbc100]

Sure seems like some kind of bug in github actions. The action claims to be cancelled, which is something that normally happens when it times out, or some other external event occurs. IIUC that should never happen due to something that is running inside the action itself.

Maybe open an issue with github?

[sbc100]

There some interesting timing information in the logs:

2023-04-13T19:08:00.4130939Z checkStackCookie: 0
2023-04-13T19:08:37.2706943Z ##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.

Note the 37 seconds between the last emscripten message and the The runner has received a shutdown signal message. It seems it takes 37 seconds.. is that meaningfull? How long do github actions normally wait for more output? If the problem is node not exiting, why would that not also happen when you run locally?

[Shaikh-Ubaid]

Please see CI: Use node-12.22.12-64bit and https://github.com/Shaikh-Ubaid/lfortran/actions/runs/4696630413/jobs/8326896817?pr=14. It seems to work with node 12.22.12 (installed via nvm).

I observed that on my local system, my ram utilization goes upto 14Gbs with node 14.18.2, slight over 14Gbs with node 12.22.12, close to 15Gbs with node 15.14.0 and close to 19.9Gbs with node 18.15.0, where my total ram capacity is 20Gbs.

From https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources, it looks like the ram capacity available to Linux machines is 7Gbs and SSD space is 14Gbs.

I guess that it works with node 14.18.2 and 12.22.12 by utilizing swap space in SSD, but does not work with newer node versions as they seem to require more ram memory to work and GitHub might be halting the process in case of no memory space available.

[sbc100]

So it seems like memory issue and the various node versions seem to have different memory usage characteristics. If this program expected to use a lot of memory? Are you using a large webassembly memory? Are you using a grow-able memory?

[Shaikh-Ubaid]

If this program expected to use a lot of memory? Are you using a large webassembly memory?

I built the project without emscripten, that is using cmake and it does not use large memory. My ram usage despite multiple runs stays (almost) the same (below 4Gbs).

Are you using a grow-able memory?

We did not pass any emscripten related flags. I specifically added the runtime_debug and assertions flag here Add debugging flag (src/lfortran/tests/CMakeLists.txt) as suggested in #829 (comment).

Our code uses malloc and free. Please, could you share which emscripten flags I should be passing so that (hopefully) the tests on running with node use memory size similar to the one built without emscripten? Also, do you know why the tests on execution with node are consuming large ram memory? Since we did not pass any emscripten flags, is using more memory a default behaviour?

[dschuff]

How are you measuring memory? V8 (in node) uses memory in a very different way than a natively-compiled C/C++ version of your app. In particular V8 uses several security isolation techniques that involve reserving large amounts of address space without actually using/committing it. You want to be comparing the working/resident/committed size (the wording varies depending on the OS). It also depends on how large you set the initial linear memory size for wasm. But running in node will probably always cost more memory overall than a native version.

[sbc100]

Certainly seems like like an OOM error yes. Although I can't see why would happen after the node process seems to be done. Is node somehow allocating more memory on exit?

Does you code use threads?

The actually memory used by emscripten without any extra flags should be 16Mb .. since by default that is the size of wasm memory and it is not growable. If you test code requires multiple Gb of heap I'm not sure how it could ever have run with emscripten defaults.

[Shaikh-Ubaid]

How are you measuring memory?

I open system monitor application parallely on my local system. While running the tests on my local system, I am observing the memory utilization shown in the resources tab of the system monitor.