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.

Sign up for GitHub

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

Jump to bottom

[v10.x] Revert buffer crash #23875

Closed
wants to merge 3 commits into from
Closed

[v10.x] Revert buffer crash #23875

wants to merge 3 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 25, 2018

Patch for the node10 branch

This reverts 2555cb4 that introduced a native crash, and adds the regression test authored for (#23795)

Refs: #23795
Refs: #22129
Fixes: #23668

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

chichiwang and others added 3 commits October 20, 2018 08:10
@chichiwang @addaleax
src: remove function hasTextDecoder in encoding.js
8b431ad 
also... return TextDecoder directly from factories

PR-URL: nodejs#23625
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@refack
Revert "src: remove calls to deprecated v8 functions (IntegerValue)"
72ffab8 
This reverts commit 2555cb4.
@refack
buffer: test fix crash for invalid index types
d39da71
@nodejs-github-bot
Copy link
Collaborator

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1365/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v10.x labels Oct 25, 2018
@refack refack added regression Issues related to regressions. lts-watch-v10.x labels Oct 25, 2018
@refack refack changed the title Revert buffer crash [v10.x] Revert buffer crash Oct 25, 2018
@refack refack added the buffer Issues and PRs related to the buffer subsystem. label Oct 25, 2018
@refack
Copy link
Contributor Author

refack commented Oct 25, 2018

/CC @nodejs/release

@refack refack mentioned this pull request Oct 25, 2018
Closed
3 tasks
@addaleax
Copy link
Member

Can you explain how this is preferable over #23795?

The main difference seems to be that this re-introduces a bunch of calls to deprecated V8 functions.

@addaleax
Copy link
Member

Another difference is that this way, we actually do perform the copy operation if the coercion throws an exception. I think that’s definitely a bug (and, after all, the reason why these calls are deprecated).

I’ve updated the test in #23795 to account for that.

jasnell
jasnell requested changes Oct 25, 2018
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference is for the fix in #23795

@refack
Copy link
Contributor Author

refack commented Oct 25, 2018

Can you explain how this is preferable over #23795?

I've submitted this just as an alternative approach. IMHO reverting 2555cb4 is the safer choise for the LTS branch.

I defer the decision to @nodejs/release

addaleax
addaleax requested changes Oct 25, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like James, I think this is not the preferable approach, in particular because it keeps the bug around that the deprecation of this methods is supposed to prevent.

@jasnell
Copy link
Member

jasnell commented Oct 25, 2018

@nodejs/tsc ... We have two competing PRs addressing the same issue. We need a resolution. This PR is one option, #23795 is the other.

@BridgeAR
Copy link
Member

Just by looking at the two different solutions I prefer #23795.

@richardlau
Copy link
Member

@nodejs/tsc ... We have two competing PRs addressing the same issue. We need a resolution. This PR is one option, #23795 is the other.

I think the TSC only needs to get involved if this can't be determined within the Release WG who has final authority over release content: https://github.com/nodejs/Release/blob/master/GOVERNANCE.md.

ryzokuken
ryzokuken previously requested changes Oct 25, 2018
View reviewed changes
Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot possibly approve this, since this introduces the use of multiple functions that'll be deprecated in V8 soon. Could you please not use those? I think others will like this one much better without them too.

mcollina
mcollina requested changes Oct 25, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer #23795

@jasnell
Copy link
Member

jasnell commented Oct 25, 2018

@richardlau

I think the TSC only needs to get involved if this can't be determined within the Release WG who has final authority over release content

FWIW, I am a member of the Release WG and I tagged this tsc-review only to make sure folks were aware that there is a contention.

@richardlau
Copy link
Member

With regards to v10.x:

https://github.com/nodejs/node/blob/f14274714d00bb5b58482bef15e84cdda9237910/COLLABORATOR_GUIDE.md#when-breaking-changes-actually-break-things

  • If a breaking commit does accidentally land in a Current or LTS branch, an attempt to fix the issue will be made before the next release; If no fix is provided then the commit will be reverted.

2555cb4 is the breaking commit that landed in v10.x (v10.10.0, although the changelog has this as c637d41b9d).

#23795 is an attempt to fix the issue while #23875 is a revert.

#23795 has a number of approvals, so IMHO can land as a fix.

The alternative is to revert (#23875) and then immediately re-backport #22129 including the changes from #23795.

@refack
Copy link
Contributor Author

refack commented Oct 29, 2018

https://ci.nodejs.org/job/node-test-pull-request/18207/

@refack
Copy link
Contributor Author

refack commented Oct 29, 2018

Hello @ryzokuken and thank you for explaining your review.

I cannot possibly approve this, since this introduces the use of multiple functions that'll be deprecated in V8 soon.

Since this PR is targeted at v10.x branch only, IMHO this argument is less urgent since there is no V8 bump planned for that branch (especially not a bump that will be semver-major). So as I see your point about not approving, but you did abstain, but chose to to request changes which is more than just approving.

Could you please not use those? I think others will like this one much better without them too.

The motivation of this PR is to revert a change that escalated a silent but into a hard crash. IMHO the simplest and safest solution is to revert that change. Specifically since it is a future-compatibility refactoring (which is of less relevance for the LTS branch), and does not introduce new functionality or fixes a bug.

@refack
Copy link
Contributor Author

refack commented Oct 29, 2018 •
edited
Loading

P.S. As I stated in #23875 (comment) I defer the choice of this over #23795 to the release team, so I'm not sure what is the meaning adding more unexplained ❌s to an already blocked PR.
AFAIR this is unprecedented, and IMHO this escalates the discussion beyond the merits of the PR.

@ryzokuken
Copy link
Contributor

@refack I see your point, you're right. I'll dismiss my review.

@ryzokuken ryzokuken dismissed their stale review October 29, 2018 17:44

Convinced otherwise

@refack
Copy link
Contributor Author

refack commented Oct 29, 2018

@refack I see your point, you're right. I'll dismiss my review.

Thank you, that is very much appreciated 🎩

@refack
Copy link
Contributor Author

refack commented Oct 30, 2018

superseded by #23795

@refack refack closed this Oct 30, 2018
@refack refack deleted the revert-buffer-crash branch November 1, 2018 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax requested changes

@jasnell jasnell jasnell requested changes

@mcollina mcollina mcollina requested changes

@ryzokuken ryzokuken ryzokuken left review comments

@bnoordhuis bnoordhuis Awaiting requested review from bnoordhuis

Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. regression Issues related to regressions.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@refack @nodejs-github-bot @addaleax @jasnell @BridgeAR @richardlau @ryzokuken @mcollina @BethGriggs @chichiwang