-
Notifications
You must be signed in to change notification settings - Fork 5.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
fix(ext/node): shared global buffer unlock correctness fix #20314
Conversation
caffbe6
to
e2298e0
Compare
// Try to read again if the underlying stream resource | ||
// changed. This can happen during TLS upgrades (eg. STARTTLS) | ||
if (ridBefore != this[kStreamBaseField]!.rid) { | ||
return this.#read(); | ||
} | ||
|
||
buf = new Uint8Array(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this line for clarity
1436ab0
to
8d1942b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me for the changes, didn't analyse the code around it
bufLocked = true; | ||
// Lock safety: We must hold this lock until we are certain that buf is no longer used | ||
// This setup code is a little verbose, but we need to be careful about buffer management | ||
let buf, locked; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I'd prep locked to false. I do not know if that helps V8 with optimisation but it might. Also makes TS understand it even in a JS file.
fef71a7
to
46ce46f
Compare
|
||
server.close(() => { | ||
p.resolve(); | ||
finished.resolve(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that in the old code, p2 was guaranteed to fire before p so there wasn't much sense in waiting on both.
Fixes #19831 |
if (bufLocked) { | ||
// Already locked, allocate | ||
buf = new Uint8Array(SUGGESTED_SIZE); | ||
} else { | ||
// Not locked, take the buffer + lock | ||
buf = BUF; | ||
locked = bufLocked = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't the code be much simpler if you always allocate buf = new Uint8Array(SUGGESTED_SIZE);
? locked, bufLocked
will be deemed useless.
Is the current shared buffer motivated only by a performance gain? @mmastrac
The fix for #20188 was not entirely correct -- we were unlocking the global buffer incorrectly. This PR introduces a lock state that ensures we only unlock a lock we have taken out.
The fix for #20188 was not entirely correct -- we were unlocking the global buffer incorrectly. This PR introduces a lock state that ensures we only unlock a lock we have taken out.