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

What could happend, If you update ICU in Node #483

Closed
mabels opened this issue Oct 9, 2019 · 9 comments
Closed

What could happend, If you update ICU in Node #483

mabels opened this issue Oct 9, 2019 · 9 comments
Assignees
Labels

Comments

@mabels
Copy link

mabels commented Oct 9, 2019

In these two node version of the same major the icu has been updated from
Icu major 62.1 to 64.2.

[abels@rose node-v10.14.2]$ ./node -e 'console.log(process.versions)'
{ http_parser: '2.8.0',
  node: '10.14.2',
  v8: '6.8.275.32-node.45',
  uv: '1.23.2',
  zlib: '1.2.11',
  ares: '1.15.0',
  modules: '64',
  nghttp2: '1.34.0',
  napi: '3',
  openssl: '1.1.0j',
  icu: '62.1',
  unicode: '11.0',
  cldr: '33.1',
  tz: '2018e' }
[abels@rose node-v10.16.2]$ ./node -e 'console.log(process.versions)'
{ http_parser: '2.8.0',
  node: '10.16.2',
  v8: '6.8.275.32-node.54',
  uv: '1.28.0',
  zlib: '1.2.11',
  brotli: '1.0.7',
  ares: '1.15.0',
  modules: '64',
  nghttp2: '1.34.0',
  napi: '4',
  openssl: '1.1.1c',
  icu: '64.2',
  unicode: '12.1',
  cldr: '35.1',
  tz: '2019a' }

And this causes no problems in the first place but in a special case the result was very unlikely.
To prevent this I recommend to not update icu major version in the same node release stream. And to check if the icudt64l.dat file is still compatible.
What happened in this very unpleasant special case?
A standard node is compiled and distributed with the small translation set where these dat file is only ~3MB in size. But if you have an application which need the full set you could use this “https://www.npmjs.com/package/full-icu” or compile by your self and extract the “icudt64l.dat”. If you use the right versions to created the “icudt64.dat” everything is working like expected. But if you use the wrong version node not starting at all:

[abels@rose ~]$ nvm use v10.14.2
Now using node v10.14.2 (npm v6.4.1)
[abels@rose ~]$ node --icu-data-dir=/home/abels/.nvm/versions/node/v10.16.2/lib/node_modules/full-icu
node: could not initialize ICU (check NODE_ICU_DATA or --icu-data-dir parameters)
[abels@rose ~]$ node --icu-data-dir=/home/abels/.nvm/versions/node/v10.14.2/lib/node_modules/full-icu
>
[abels@rose ~]$ nvm use v10.16.2
Now using node v10.16.2 (npm v6.9.0)
[abels@rose ~]$ node --icu-data-dir=/home/abels/.nvm/versions/node/v10.14.2/lib/node_modules/full-icu
node: could not initialize ICU (check NODE_ICU_DATA or --icu-data-dir parameters)
[abels@rose ~]$ node --icu-data-dir=/home/abels/.nvm/versions/node/v10.16.2/lib/node_modules/full-icu
>

This not starting behavior is ok if you control the node environment, mean you run your application by a node version chosen from you.
But if you run in an “AWS Lambda” you are not in control of the node version, and than if “AWS” updates the node version which is intendent all your existing lambda with a reference to the fullicu stop working and need a redeployment of a unchanged software. This is not what you expect from any software, during the update a minor version of node.

@BethGriggs
Copy link
Member

/cc @nodejs/i18n @srl295

@srl295
Copy link
Member

srl295 commented Oct 9, 2019

Some notes:

  • the 'could not initialize' was expected, to avoid continuing on with unexpected content.
  • v8 often requires a new major ICU version to support new features
  • we have backported the ICU major bump before
  • The argument to --icu-data-dir could contain both icudt64l.dat and icudt62l.dat and node will use the appropriate item.

And:

  • with Make full-icu the default node#29522 this is less of an issue, because ICU with full data will continue to function as expected even with --icu-data-dir
  • I expect the full-icu module to not be commonly used as before now that the default is full-icu.

@mabels
Copy link
Author

mabels commented Oct 10, 2019

Some notes:

  • the 'could not initialize' was expected, to avoid continuing on with unexpected content.
  • v8 often requires a new major ICU version to support new features
  • we have backported the ICU major bump before
  • The argument to --icu-data-dir could contain both icudt64l.dat and icudt62l.dat and node will use the appropriate item.

And:

  • with nodejs/node#29522 this is less of an issue, because ICU with full data will continue to function as expected even with --icu-data-dir
  • I expect the full-icu module to not be commonly used as before now that the default is full-icu.

That are all good arguments and facts but as long some major cloud provider aren't switched to full-icu the update of a node minor version should not cause serious trouble.
Node is not V8 neither chrome and my opion on this is, node has to avoid should trouble in minor version releases.
It's more like the linux-kernel does an update of a system call during a bugfix in a stable version which causes chrome to crash, this is an unexpected behavoir.
And this Issue is more about how to avoid such problems in future node minor versions as long not all nodejs running major enviroments are switched to full-icu.

@BridgeAR
Copy link
Member

@mabels we try hard not to break anyone's code. The full-icu will be included in the Node.js binary in future releases. Thus, the cloud provider would also automatically use that (if they do not build their own Node.js version that explicitly excludes full-icu). So hopefully, this will prevent all of these issues in the future.

@srl295
Copy link
Member

srl295 commented Oct 10, 2019

@mabels yes, and I am sorry to hear things aren't working. That's why I got started with this integration, to make things easier. There are, however, a lot of trade offs.

I think the real short answer is that calling npm install full-icu one time is not a good way to install localization data for a changing environment such as AWS Lambda, and I noted this in nodejs/full-icu-npm#31 .

The issue is --icu-data-dir — this is intended to provide very specific configuration data for your exact version of node. I think the technically best solution, from the configuration perspective, is to re-run npm install full-icu every single time. But that would slow down each invocation.

In a way, the root cause is that you have installed it globally once (with a specific nvm directory), and (understandably!) assume that it could continue to be used. This is not a guarantee that was ever made. I'm sorry if npm i full-icu, as complicated as it is, made this seem like it would work.

It's more like the linux-kernel does an update of a system call during a bugfix in a stable version which causes chrome to crash, this is an unexpected behavoir.

I think it's more like having code expecting a system call to be implemented at a specific memory address, and then with a bugfix the memory address changes.

Moving forward

  • You could ask your cloud provider today to build their node with --with-intl=full-icu --download=all. That would improve everyone's result today. Node only changed the default, this option has been available since v0.12

  • We can improve the documentation in node: in the readme, in the command line options such as --icu-data-dir. We can improve the online and runtime documentation in the full-icu module with a warning about this use case.

@srl295
Copy link
Member

srl295 commented Oct 10, 2019

It's always been the case that uploading the result of npm i full-icu to a deployment provider is not recommended.

@mabels
Copy link
Author

mabels commented Oct 11, 2019

@mabels yes, and I am sorry to hear things aren't working. That's why I got started with this integration, to make things easier. There are, however, a lot of trade offs.

I think the real short answer is that calling npm install full-icu one time is not a good way to install localization data for a changing environment such as AWS Lambda, and I noted this in unicode-org/full-icu-npm#31 .

The issue is --icu-data-dir — this is intended to provide very specific configuration data for your exact version of node. I think the technically best solution, from the configuration perspective, is to re-run npm install full-icu every single time. But that would slow down each invocation.

In a way, the root cause is that you have installed it globally once (with a specific nvm directory), and (understandably!) assume that it could continue to be used. This is not a guarantee that was ever made. I'm sorry if npm i full-icu, as complicated as it is, made this seem like it would work.

It's more like the linux-kernel does an update of a system call during a bugfix in a stable version which causes chrome to crash, this is an unexpected behavoir.

I think it's more like having code expecting a system call to be implemented at a specific memory address, and then with a bugfix the memory address changes.

Moving forward

  • You could ask your cloud provider today to build their node with --with-intl=full-icu --download=all. That would improve everyone's result today. Node only changed the default, this option has been available since v0.12
  • We can improve the documentation in node: in the readme, in the command line options such as --icu-data-dir. We can improve the online and runtime documentation in the full-icu module with a warning about this use case.

I agree with this. First there are good reasons to your software to make use of the full-icu package. But if your are not controlling how the node runtime is build/provided than you need to mitigate between do not use Intl or switch the cloud provider. And a mitigation could be to use your own .dat files but this comes with not obviously backscatter. And to push your cloud provider is like pushing a truck, it takes time and you are not knowning if this is getting to a end. On the other hand you have get your own software to run.
And I just want that the node runtime respects and addresses this odd behavior until the big trucks like "AWS" has updated there runtime to full-icu.
If node continues with the current icu updare policy the developers has to find a other solution as I did
which puts the node internal Intl api in question.

@srl295
Copy link
Member

srl295 commented Nov 14, 2019

But if your are not controlling how the node runtime is build/provided than you need to mitigate between do not use Intl or switch the cloud provider. And a mitigation could be to use your own .dat files but this comes with not obviously backscatter. And to push your cloud provider is like pushing a truck, it takes time and you are not knowning if this is getting to a end. On the other hand you have get your own software to run.
And I just want that the node runtime respects and addresses this odd behavior until the big trucks like "AWS" has updated there runtime to full-icu.
If node continues with the current icu updare policy the developers has to find a other solution as I did
which puts the node internal Intl api in question.

If the provider isn't going to mitigate this themselves (and again, the best solution I think is for them to start building with --with-intl=full-icu now) then I don't think there's any other positive action for Node to take… other than holding off on ICU updates in prior versions as requested here ( see for example nodejs/node#30232 (comment) ).

For LTS releases past 13, I think updating should be fine because the ICU data will be 'built in'.

@richardlau @mabels with that understanding, can we close this issue?

@BridgeAR
Copy link
Member

Closing as suggested. @mabels @richardlau please reopen / leave a comment if there's anything left that should be looked at closer.

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

No branches or pull requests

4 participants