-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
Comments
Do you know of any open-source application/project that may be affected by this? |
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. |
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. |
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 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 |
https://www.npmjs.com/package/jest-serializer (20.4 million downloads per week) directly uses There are also a number of cases like this to be found from a github search: 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 |
I think that would be an ideal outcome here, yes. |
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. |
@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. |
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. |
@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:
|
V8 update landed and the release notes will contain a paragraph about this. See also 0854fce |
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... |
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 |
Even at semver-major, it's weird to have a change that makes it impossible to do a rolling upgrade... |
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. |
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 to15
.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))
--> dataNode.js 17.x -->
v8.deserialize(data)
--> throws due to unsupported versionThe 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.
The text was updated successfully, but these errors were encountered: