fix: ensure transferred ArrayBuffer are Transferable #6251
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6251 +/- ##
=========================================
Coverage 80.31% 80.31%
=========================================
Files 202 202
Lines 19543 19543
Branches 1169 1169
=========================================
Hits 15695 15695
Misses 3820 3820
Partials 28 28 |
Performance Report✔️ no performance regression detected Full benchmark results
|
nflaig
reviewed
Jan 5, 2024
| const isMarkedAsUntransferable = unknownWorker["isMarkedAsUntransferable"]; | ||
| // Can be updated to direct access once minimal version of node is 21 | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-call | ||
| if (isMarkedAsUntransferable && isMarkedAsUntransferable(buffer)) { |
There was a problem hiding this comment.
Isn't the buffer always untransferable or why do we need this check? Based on nodejs/node#47604 it would previously (node 20) just silently copy the buffer while with node 21 it throws an error. The problem seems to be related to @chainsafe/bls-keystore and how the keystore buffer is created.
Upgrading @chainsafe/bls-keystore fixes the issue for me since with v3 it uses Unit8Array instead of Buffer. Will do some performance testing with new version and open PR for that
There was a problem hiding this comment.
Right, it might not be transferred at all with node20.
If the new @chainsafe/bls-keystore offers equivalent or better perf that's definitely the better option.
There was a problem hiding this comment.
Could you try #6253 on your machine? At least on linux there are no issues running unit tests on node 21
There was a problem hiding this comment.
I can confirm that it works on osx with node 21
|
Closing this PR for now since #6253 resolved the issue |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
test:unitdo not pass on nodejs starting version 21 due to a change inArrayBuffertransferabilityDescription
Node version 21 introduces changes to the way ArrayBuffer transferability are dealt with. Specifically some of current usage now is not transferable anymore and breaks usage.
To deal with this, a new API has been introduced in node 21 to detect if an object has been marked as not transferable. This API is used (when available) to do a copy of the underlying buffer instead of directly transferring it.
Performance in this case is still assumed to be better than going through full (de)/serialization.