os: modify os.tmpdir() to judge more recommended order in windows #5083
Conversation
If someone(e.g. amdin) want to use an old NT command script or has installed old windows in another dirve, then %SystemRoot% would be more suitable. And %windir is also used for old windows compatibility. See: nodejs#3407
|
Also, usually windows users set their temporary folder with variable TEMP, not TMP. see: |
|
On Unix systems, It would probably be best to just have a different function on Windows entirely. |
| process.env.TEMP || | ||
| (process.platform === 'win32' ? 'c:\\windows\\temp' : '/tmp'); | ||
| process.env.TMP || | ||
| (process.platform === 'win32' ? (process.env.systemroot || process.env.windir) + "\\temp" : '/tmp'); |
There was a problem hiding this comment.
Style: Long line.
Implementation: Casing? I thought it was SystemRoot and WinDir.
There was a problem hiding this comment.
Basically, windows doesn't care character case of env variables.
And standard expression is SystemRoot and windir
http://msdn.microsoft.com/en-us/library/windows/desktop/aa384187(v=vs.85).aspx
I'll change code style and character cases, and request pull again.
It will be a different function on Windows entirely
Thanks
I'm admittedly not 100% sure what the standard is (if there is one) but glibc looks for $TMPDIR first, then falls back to /tmp. It never considers $TMP. EDIT: As another data point, musl libc is hard-coded to only use /tmp. |
|
I know it's subtle and I'm afraid bothering, but may I have a opinion about commit b7f6418, @isaacs @bnoordhuis ? |
|
The UNIX side of things LGTM. @piscisaureus or @sblom should sign off on the Windows part. |
|
@piscisaureus @bnoordhuis Thanks! I have already signed the CLA. "Suwon Chae" is my full name. |
| if (isWindows) { | ||
| return process.env.TEMP || | ||
| process.env.TMP || | ||
| (process.env.SystemRoot || process.env.windir) + "\\temp"; |
There was a problem hiding this comment.
Looking at it again, it seems that if neither %SystemRoot% or %windir% is set, it'll return 'undefined\\temp'.
/cc @piscisaureus
There was a problem hiding this comment.
True that. However when that happens you have a much bigger problem. Node won't even start any more :)
|
Okay, landed in 120e5a2. Thanks! |
The role of `this.server` is now split between `this._server` and `this.server`. Where the first one is used for counting active connections of `net.Server`, and the latter one is just a public API for users' consumption. The reasoning for this is simple, `TLSSocket` instances wrap `net.Socket` instances, thus both refer to the `net.Server` through the `this.server` property. However, only one of them should be used for `net.Server` connection count book-keeping, otherwise double-decrement will happen on socket destruction. Fix: nodejs#5083 PR-URL: nodejs/node#5262 Reviewed-By: James M Snell <jasnell@gmail.com>
The role of `this.server` is now split between `this._server` and `this.server`. Where the first one is used for counting active connections of `net.Server`, and the latter one is just a public API for users' consumption. The reasoning for this is simple, `TLSSocket` instances wrap `net.Socket` instances, thus both refer to the `net.Server` through the `this.server` property. However, only one of them should be used for `net.Server` connection count book-keeping, otherwise double-decrement will happen on socket destruction. Fix: nodejs#5083 PR-URL: nodejs/node#5262 Reviewed-By: James M Snell <jasnell@gmail.com>
The role of `this.server` is now split between `this._server` and `this.server`. Where the first one is used for counting active connections of `net.Server`, and the latter one is just a public API for users' consumption. The reasoning for this is simple, `TLSSocket` instances wrap `net.Socket` instances, thus both refer to the `net.Server` through the `this.server` property. However, only one of them should be used for `net.Server` connection count book-keeping, otherwise double-decrement will happen on socket destruction. Fix: nodejs#5083 PR-URL: nodejs/node#5262 Reviewed-By: James M Snell <jasnell@gmail.com>
If someone(e.g. amdin) want to use an old NT command script
or has installed old windows in another dirve,
then %SystemRoot% would be more suitable.
And %windir is also used for old windows compatibility.
See: #3407