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

Heads up: v8 9.9 updates the ValueSerializer/ValueDeserializer version #42192

Closed
jasnell opened this issue Mar 2, 2022 · 15 comments
Closed

Heads up: v8 9.9 updates the ValueSerializer/ValueDeserializer version #42192

jasnell opened this issue Mar 2, 2022 · 15 comments
Labels
v8 engine Issues and PRs related to the V8 dependency. v8 module Issues and PRs related to the "v8" subsystem.

Comments

@jasnell
Copy link
Member

jasnell commented Mar 2, 2022

Just a heads up that V8 9.9 has updated the version number of the serialization format used by v8::ValueSerializer to 14, and has updated the version again in v8 main branch to 15.

This will cause issues as older versions of Node.js will not be able to successfully deserialize data serialized using V8 9.9.

Specifically, if we assume that Node.js 18.x will be bumped to V8 9.9, then the following flow will not work:

Node.js 18.x --> v8.serialize(new Uint8Array(10)) --> data
Node.js 17.x --> v8.deserialize(data) --> throws due to unsupported version

The deserializer is not forwards-compatible.

/cc @addaleax @targos

@lucacasonato ... Just a heads up for you here as well. I know y'all haven't yet implemented the v8.serialize/v8.deserialize APIs in the Deno node module but once you do get around to that these version bumps will likely impact deno as well.

@jasnell jasnell added the v8 engine Issues and PRs related to the V8 dependency. label Mar 2, 2022
@Mesteery Mesteery added the v8 module Issues and PRs related to the "v8" subsystem. label Mar 3, 2022
@targos
Copy link
Member

targos commented Mar 3, 2022

Do you know of any open-source application/project that may be affected by this?

@addaleax
Copy link
Member

addaleax commented Mar 3, 2022

The deserializer is not forwards-compatible

Yeah, that’s a known property of how it works. It’s just something to be called out in the release notes, I assume? Breaking changes happen in major releases, that should be okay in this case, and it doesn’t seem like there’s anything else that’s actionable here for us.

@kentonv
Copy link

kentonv commented Mar 3, 2022

It seems entirely plausible that someone out there is using v8 serialization to store data in databases or transmit RPCs. That seems like an intended use case. There is a valid expectation that v8 serialization is backwards-compatible.

During a rolling update of multiple servers, these use cases would temporarily break when old servers receive data generated by new servers. In other words, it becomes impossible to update a large cloud service without downtime. Also, rollbacks would be impossible if data is stored to disk.

I wouldn't know if anything built on Node is affected, but Cloudflare Workers is affected by this and we're going to have to patch V8 to get around it.

@jasnell
Copy link
Member Author

jasnell commented Mar 3, 2022

The reason we (cloudflare) caught it is that one of our products stores the serialization format directly. We use a rolling update mechanism that means when a v8 update happens not all of our systems are on the same v8 version at the same time during the rollout and sometimes we end up having to roll back mid deployment. If, during that time, someone happens to serialize on the newer version before things roll back, or the data is accessed from a system that is still on the older version, we can run into issues. There's nothing directly in Node.js that is similarly affected, and we do document that the v8 module is specific to the version of v8 that is compiled into the binary, but it is absolutely conceivable that folks could have similar problems in a phased rollout of Node.js 18.x, so I think it absolutely should be called out in the release notes for whenever we update v8 with an updated serialization version. We should also take care not to bump v8's between majors if there are serialization version updates. We (again, cloudflare) wanted to make sure that the issue was at least highlighted just in case others stumbled across the same issue.

The patch we've applied in the workers case allows us to explicitly pin the deserializer to a particular version but we've also asked the v8 team to consider other solutions -- such as potentially allowing the serialization version to be set in the v8::ValueSerializer.

@jasnell
Copy link
Member Author

jasnell commented Mar 3, 2022

@targos:

Do you know of any open-source application/project that may be affected by this?

https://www.npmjs.com/package/jest-serializer (20.4 million downloads per week) directly uses v8.serialize/v8.deserialize

There are also a number of cases like this to be found from a github search:

https://github.com/duongkimseng/gatsby-docker-sample/blob/e565bb87b2c1c31b68d1fea6192dd29b583ac49e/packages/gatsby/src/redux/persist.js

const useV8 = Boolean(v8.serialize)
const [serialize, deserialize, file] = useV8
  ? [v8.serialize, v8.deserialize, `${process.cwd()}/.cache/redux.state`]
  : [jsonStringify, jsonParse, `${process.cwd()}/.cache/redux-state.json`]

const readFromCache = () => deserialize(fs.readFileSync(file))

const writeToCache = contents => {
  fs.writeFileSync(file, serialize(contents))
}

Also, it looks like Parcel is using v8.serialize/v8.deserialize to store cached, parsed ASTs to disk in some cases.

@addaleax
Copy link
Member

addaleax commented Mar 3, 2022

such as potentially allowing the serialization version to be set in the v8::ValueSerializer.

I think that would be an ideal outcome here, yes.

@targos
Copy link
Member

targos commented Mar 4, 2022

In 4696a55 I followed a recommendation from a V8 team member to remove the assertion that the new V8 serializes to the same bytes as the old one. Should I actually add a similar test case whose purpose is to let us know that the format has changed? Otherwise we might miss future changes and not call them out in the release notes.

@kentonv
Copy link

kentonv commented Mar 4, 2022

@targos We're doing exactly that in the Cloudflare Workers codebase -- a test that will fail whenever there is a new V8 serialization version, thus alerting us that we should investigate.

@marjakh
Copy link
Contributor

marjakh commented Mar 9, 2022

Hi, being the V8 team member referred to by the above comment; if you do need forwards-compatibility in addition to backwards-compatibility, then it's totally fine to have a test which alerts when the version number changes.

For clarity (also towards us, when we do break the test), the test could be just checking the version number byte, with a clarifying comment. Then when we notice that test starts failing on our waterfall, we can already alert you at that point, if the comment contains instructions how to do that.

@targos
Copy link
Member

targos commented Mar 17, 2022

@marjakh I pushed this commit to the 9.9 update PR: targos@c975dc1

In case of new serialization format, it will error with a message like that:

./node --expose-internals test/parallel/test-v8-serdes.js
node:assert:123
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: New serialization format.

    This test is expected to fail when V8 changes its serialization format.
    When that happens, the "desStr" variable must be updated to the new value
    and the change should be mentioned in the release notes, as it is semver-major.

    Consider opening an issue as a heads up at https://github.com/nodejs/node/issues/new

    at Object.<anonymous> (/Users/targos/git/nodejs/v8-next-update/test/parallel/test-v8-serdes.js:196:10)
    at Module._compile (node:internal/modules/cjs/loader:1097:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1151:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at node:internal/main/run_main_module:17:47 {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: 'ff0e6f2203666f6f5e007b01',
  expected: 'ff0e6f2203666f6f5e007b011',
  operator: 'deepStrictEqual'
}

Node.js v18.0.0-pre

@targos
Copy link
Member

targos commented Apr 12, 2022

V8 update landed and the release notes will contain a paragraph about this. See also 0854fce

@targos targos closed this as completed Apr 12, 2022
@kentonv
Copy link

kentonv commented Apr 13, 2022

This was closed, but how is Node going to address the compatibility problem? When someone creates a serialized value with a newer version of node and tries to read it with an older version, they'll experience a failure. Probably going to be common in large-scale server farms where not all the servers can be updated simultaneously...

@jasnell
Copy link
Member Author

jasnell commented Apr 13, 2022

Currently the plan is to treat such changes as semver-major and to highlight them in the notable changes on the release notes. I'd personally prefer adding the additional support for setting the version explicitly but we generally try not to float patches like that in node.js

@kentonv
Copy link

kentonv commented Apr 13, 2022

Even at semver-major, it's weird to have a change that makes it impossible to do a rolling upgrade...

@jasnell
Copy link
Member Author

jasnell commented Apr 14, 2022

Yeah, I agree it's a challenge. The best bet here is still to try to convince v8 to update the API. I'm considering opening a PR that does so and just seeing how the v8 folks react.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency. v8 module Issues and PRs related to the "v8" subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants