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

Can --trace-atomics-wait be removed #42982

Closed
syg opened this issue May 5, 2022 · 10 comments · Fixed by #52747
Closed

Can --trace-atomics-wait be removed #42982

syg opened this issue May 5, 2022 · 10 comments · Fixed by #52747
Labels
feature request Issues that request new features to be added to Node.js. never-stale Mark issue so that it is never considered stale

Comments

@syg
Copy link
Contributor

syg commented May 5, 2022

What is the problem this feature will solve?

The V8 API hook (SetAtomicsWaitCallback) to support --trace-atomics-wait is fairly complex and adds maintenance burden. Further the only user of the API is node, and node only uses a subset of the functionality, and only for diagnostic purposes.

Is it feasible to remove this diagnostic functionality? Judging by the original commit message the original motivation was maybe to use the hooks to build a deadlock detection system, not just printf diagnostics. It's been 4 years since then, and if no such system has been prototyped and it's not on the roadmap anytime soon, I'd like to deprecate and remove the V8 API.

cc @addaleax

What is the feature you are proposing to solve the problem?

Remove --trace-atomics-wait

What alternatives have you considered?

No response
@syg syg added the feature request Issues that request new features to be added to Node.js. label May 5, 2022
@addaleax
Copy link
Member

addaleax commented May 6, 2022

@syg I am personally fine with anything here. --trace-atomics-wait has been useful for me in the past occasionally, and the thing about it is that it’s useful specifically in situations in which it can be hard to debug with other tools, but if you feel that it’s not worth it, sure, drop it.

@syg
Copy link
Contributor Author

syg commented May 6, 2022

@syg I am personally fine with anything here. --trace-atomics-wait has been useful for me in the past occasionally, and the thing about it is that it’s useful specifically in situations in which it can be hard to debug with other tools, but if you feel that it’s not worth it, sure, drop it.

Given the current functionality of --trace-atomics-wait tracing the Atomics.wait events (not using the handle to interrupt waits), is it possible to do that in userland? I guess the problem is that on a failure to wait, regardless of whether you re-read the address after the fact or buffer the address beforehand, there's still no guarantee if that value is what caused it to fail?

@syg
Copy link
Contributor Author

syg commented May 6, 2022

Another option here is to keep the pre-hook needed for --trace-atomics-wait, remove the other hooks, as well as the handle. The handle is probably 70% of the complexity I want to remove.

kvakil added a commit to kvakil/node that referenced this issue Aug 2, 2022
@kvakil
doc,worker: deprecate --trace-atomics-wait
8ddb975 
V8 has asked if it possible to remove the functionality underlying
`--trace-atomics-wait`. Let's start with a documentation-only
deprecation.

Refs: nodejs#42982
@kvakil kvakil mentioned this issue Aug 2, 2022
Merged
@kvakil
Copy link
Contributor

kvakil commented Aug 2, 2022

I created #44093 to start the deprecation process & for further discussion.

aduh95 pushed a commit that referenced this issue Aug 4, 2022
@kvakil
doc,worker: deprecate --trace-atomics-wait
6743d81 
V8 has asked if it possible to remove the functionality underlying
`--trace-atomics-wait`. Let's start with a documentation-only
deprecation.

PR-URL: #44093
Refs: #42982
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
danielleadams pushed a commit that referenced this issue Aug 16, 2022
@kvakil @danielleadams
doc,worker: deprecate --trace-atomics-wait
68c4e45 
V8 has asked if it possible to remove the functionality underlying
`--trace-atomics-wait`. Let's start with a documentation-only
deprecation.

PR-URL: #44093
Refs: #42982
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
ruyadorno pushed a commit that referenced this issue Aug 23, 2022
@kvakil @ruyadorno
doc,worker: deprecate --trace-atomics-wait
da7bc5a 
V8 has asked if it possible to remove the functionality underlying
`--trace-atomics-wait`. Let's start with a documentation-only
deprecation.

PR-URL: #44093
Refs: #42982
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
targos pushed a commit that referenced this issue Sep 5, 2022
@kvakil @targos
doc,worker: deprecate --trace-atomics-wait
bdda04a 
V8 has asked if it possible to remove the functionality underlying
`--trace-atomics-wait`. Let's start with a documentation-only
deprecation.

PR-URL: #44093
Refs: #42982
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Fyko pushed a commit to Fyko/node that referenced this issue Sep 15, 2022
@kvakil @Fyko
doc,worker: deprecate --trace-atomics-wait
4ec3fac 
V8 has asked if it possible to remove the functionality underlying
`--trace-atomics-wait`. Let's start with a documentation-only
deprecation.

PR-URL: nodejs#44093
Refs: nodejs#42982
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
juanarbol pushed a commit that referenced this issue Oct 10, 2022
@kvakil @juanarbol
doc,worker: deprecate --trace-atomics-wait
6def83e 
V8 has asked if it possible to remove the functionality underlying
`--trace-atomics-wait`. Let's start with a documentation-only
deprecation.

PR-URL: #44093
Refs: #42982
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
juanarbol pushed a commit that referenced this issue Oct 11, 2022
@kvakil @juanarbol
doc,worker: deprecate --trace-atomics-wait
8f6bd5a 
V8 has asked if it possible to remove the functionality underlying
`--trace-atomics-wait`. Let's start with a documentation-only
deprecation.

PR-URL: #44093
Refs: #42982
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
@kvakil @guangwong
doc,worker: deprecate --trace-atomics-wait
579b5cf 
V8 has asked if it possible to remove the functionality underlying
`--trace-atomics-wait`. Let's start with a documentation-only
deprecation.

PR-URL: nodejs/node#44093
Refs: nodejs/node#42982
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this issue Jan 3, 2023
@kvakil @guangwong
doc,worker: deprecate --trace-atomics-wait
eca370a 
V8 has asked if it possible to remove the functionality underlying
`--trace-atomics-wait`. Let's start with a documentation-only
deprecation.

PR-URL: nodejs/node#44093
Refs: nodejs/node#42982
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Jan 30, 2023
@ruyadorno ruyadorno added never-stale Mark issue so that it is never considered stale and removed stale labels Feb 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Aug 3, 2023
@aduh95
Copy link
Contributor

aduh95 commented Aug 3, 2023

We should probably runtime deprecate it to move forward with this.

@github-actions github-actions bot removed the stale label Aug 4, 2023
@marco-ippolito marco-ippolito mentioned this issue Dec 16, 2023
Closed
@marco-ippolito
Copy link
Member

Moving to runtime deprecation with #51179

jasnell pushed a commit that referenced this issue Dec 22, 2023
@marco-ippolito @jasnell
doc: runtime deprecate flag --trace-atomics-wait
b9df88a 
PR-URL: #51179
Refs: #42982
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@marco-ippolito
Copy link
Member

@syg the flag has moved to runtime deprecation, afaik it will be released in the next major (22).

@marco-ippolito marco-ippolito mentioned this issue Apr 29, 2024
Merged
@marco-ippolito
Copy link
Member

After this PR: #52747 it will be completely removed

nodejs-github-bot pushed a commit that referenced this issue May 1, 2024
@marco-ippolito
cli: move --trace-atomics-wait to eol
8f37492 
PR-URL: #52747
Fixes: #42982
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Ch3nYuY pushed a commit to Ch3nYuY/node that referenced this issue May 8, 2024
@marco-ippolito @Ch3nYuY
cli: move --trace-atomics-wait to eol
860d47c 
PR-URL: nodejs#52747
Fixes: nodejs#42982
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this issue Jun 20, 2024
@marco-ippolito @eliphazbouye
cli: move --trace-atomics-wait to eol
170646e 
PR-URL: nodejs#52747
Fixes: nodejs#42982
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
@marco-ippolito @bmeck
cli: move --trace-atomics-wait to eol
3b5d1fe 
PR-URL: nodejs#52747
Fixes: nodejs#42982
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature request Issues that request new features to be added to Node.js. never-stale Mark issue so that it is never considered stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

cli: move --trace-atomics-wait to eol marco-ippolito/node
6 participants
@syg @ruyadorno @addaleax @aduh95 @kvakil @marco-ippolito