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.

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

node.exe 8.7.0 and above does not work in Windows NanoServer container #16603

Closed
StefanScherer opened this issue Oct 30, 2017 · 31 comments · Fixed by StefanScherer/dockerfiles-windows#232
Labels
windows Issues and PRs related to the Windows platform.

Comments

@StefanScherer
Copy link

StefanScherer commented Oct 30, 2017

I have problems running the newer versions of Node 8 in a Windows Nanoserver Container:

  • Version:
    8.7.0, 8.8.0, 8.8.1

  • Platform:
    Windows

  • Subsystem:
    Windows Nanoserver Container running in Windows 10 / Windows Server 2016

To reproduce the problem, just download the node.exe of specific versions in a Windows NanoServer container:

PS C:\> docker run -it microsoft/nanoserver powershell

Then inside the container:

PS C:\> $ProgressPreference = 'SilentlyContinue'
PS C:\> iwr -useb https://nodejs.org/dist/v6.11.5/win-x64/node.exe -outfile node-6.11.5.exe
PS C:\> .\node-6.11.5.exe --version
v6.11.5
PS C:\> iwr -useb https://nodejs.org/dist/v8.6.0/win-x64/node.exe -outfile node-8.6.0.exe
PS C:\> .\node-8.6.0.exe --version
v8.6.0

Beginning with Node 8.7.0 this stops working:

PS C:\> iwr -useb https://nodejs.org/dist/v8.7.0/win-x64/node.exe -outfile node-8.7.0.exe
PS C:\> .\node-8.7.0.exe --version
PS C:\> 
PS C:\> iwr -useb https://nodejs.org/dist/v8.8.0/win-x64/node.exe -outfile node-8.8.0.exe
PS C:\> .\node-8.8.0.exe --version
PS C:\> 
PS C:\> iwr -useb https://nodejs.org/dist/v8.8.1/win-x64/node.exe -outfile node-8.8.1.exe
PS C:\> .\node-8.8.1.exe --version
PS C:\> 
@mscdex mscdex added v8.x windows Issues and PRs related to the Windows platform. labels Oct 30, 2017
@maclover7
Copy link
Contributor

cc @nodejs/platform-windows

@StefanScherer
Copy link
Author

Just tested the binaries in Windows Server 1709 images (microsoft/windowsservercore:1709 and microsoft/nanoserver:1709) and all versions 8.7.0, 8.8.0 and 8.8.1 work there.
I have to check my older Windows Server 2016 setup again why it didn't work multiple times.

@StefanScherer
Copy link
Author

OK, I ran another test on a fresh Window Server 2016 in Azure, tested with several microsoft/nanoserver:10.0.14393.1066 .. microsoft/nanoserver:10.0.14393.1770 (latest) and I can reproduce the problem. Node.exe 8.7.0 ... 8.8.1 don't work in nanoserver images for the LTS channel of Windows Server 2016.

It seems that the newer images for the Semi-annual channel of Windows Server 1709 don't have this problem.

@refack refack self-assigned this Oct 30, 2017
@refack
Copy link
Contributor

refack commented Oct 30, 2017

Ohh boy. This is not going to be fun to debug 🤦‍♂️
@StefanScherer thank you for the detailed information though 👍 Do you by any chance know where we can find something changelog like between 1770 and 1790?

@StefanScherer
Copy link
Author

@refack Oh the diff between 10.0.14393.1770 and 10.0.16299.19 = current release of Windows Server 1709 could be a little longer :-) It's not just 20 releases I guess.

@refack
Copy link
Contributor

refack commented Oct 30, 2017

@StefanScherer so it might be easier to figure this out from node's side (although I already have an obvious suspect, the bump in libuv from 1.14.1 to 1.15.0 https://github.com/libuv/libuv/blob/v1.x/ChangeLog)

@refack
Copy link
Contributor

refack commented Oct 30, 2017

P.S. node exits with 0xC0000139 {Entry Point Not Found} so it seems like we started using a new API that doesn't exists in 10.0.14393.1770

@refack
Copy link
Contributor

refack commented Oct 30, 2017

API diff:
new

AcquireSRWLockExclusive,-,-,-,kernel32.dll
DebugBreak,x,-,-,kernel32.dll
DispatchMessageA,x,-,-,user32.dll
GetMessageA,-,-,-,user32.dll
InitializeConditionVariable,-,-,-,kernel32.dll
ReleaseSRWLockExclusive,-,-,-,kernel32.dll
SetWinEventHook,x,-,-,user32.dll
SleepConditionVariableSRW,-,-,-,kernel32.dll
TranslateMessage,-,-,-,user32.dll
WakeAllConditionVariable,-,-,-,kernel32.dll
WakeConditionVariable,-,-,-,kernel32.dll

dropped:

VirtualProtect,x,-,-,kernel32.dll

@refack
Copy link
Contributor

refack commented Oct 30, 2017

Most probably SetWinEventHook

@StefanScherer
Copy link
Author

@refack I also tried older nanoserver:10.0.14393.x images and it's the same there.

But with that list of API changes in Node.js maybe @PatrickLang can have a short look ^^ and give us a hint which might cause problems?

@StefanScherer
Copy link
Author

Hm, I remember the NanoServer API scan tool. Oh I still have a Docker image of that.

FROM stefanscherer/nanoserverapiscan:onbuild
ADD https://nodejs.org/dist/v8.6.0/win-x64/node.exe node-8.6.0.exe
ADD https://nodejs.org/dist/v8.8.1/win-x64/node.exe node-8.8.1.exe
docker build -t scan .
docker run scan

But it doesn't show any errors.

@digitalinfinity
Copy link
Contributor

If @refack's assessment is correct, it's likely from libuv/libuv#1408 (since 8.7 upgraded libuv to 1.15.0)- @bzoz can you take a look, we likely need to GetProcAddress of SetWinEventHook rather than statically linking against it if the API is not present Windows NanoServer

@rgl
Copy link

rgl commented Oct 31, 2017

@refack how did you generate that API diff? I mean, what tool did you use to extract the used APIs?

@bzoz
Copy link
Contributor

bzoz commented Oct 31, 2017

@digitalinfinity I'll look into it. It definitely looks like this might be the issue here.

@gireeshpunathil
Copy link
Member

@digitalinfinity

we likely need to GetProcAddress of SetWinEventHook rather than statically linking against it if the API is not present Windows NanoServer

But if the API is not present we will fail anyways right? What additional benefit GetProcAddress provides? probably a graceful exit?

The API in question is a standard Win32 API, and the need to invoke it reflectively rather than directly, invokes question on the ability to support Nanoserver (there is no documented necessity on specialized invocation on these APIs). If we follow the suit, we may end up making similar changes to other APIs to ensure runtime reliability?

@bzoz
Copy link
Contributor

bzoz commented Oct 31, 2017

SetWinEventHook is used to detect console resize events. If it is not available, process.stdout.on('resize', ...) will stop working. Besides that Node.js will behave as normal.

@refack
Copy link
Contributor

refack commented Oct 31, 2017

how did you generate that API diff? I mean, what tool did you use to extract the used APIs?

@rgl pestudio by https://www.winitor.com/

bzoz added a commit to JaneaSystems/libuv that referenced this issue Oct 31, 2017
SetWinEventHook is not available on some Windows versions.

Fixes: nodejs/node#16603
@bzoz
Copy link
Contributor

bzoz commented Oct 31, 2017

I did a test, creating a simple int main() { SetWinEventHook(..); } reproduces this. I've opened a PR in libuv to fix this: libuv/libuv#1611

@digitalinfinity
Copy link
Contributor

@gireeshpunathil to echo @bzoz SetWinEventHook is not a critical API for Node- so the suggestion was to check for it before using it, because if it's not there, only the feature that depends on it (in this case the console resize event) would need to be off

@bzoz
Copy link
Contributor

bzoz commented Oct 31, 2017

I was able to compile Node with the libuv patch and it indeed solves this issue.

@gireeshpunathil
Copy link
Member

@digitalinfinity - thanks. My original concern is addressed by seeing a well defined document that articulates the platform differences, as well as a scanner tool to automate the dependency check. Now we seem to be well on the path for supporting NanoServer, I propose to run the node test suite in this platform and check we have a reasonable coverage, and potentially containing all the issues under one NanoServer port. What do you think?

@MylesBorins
Copy link
Contributor

libuv 1.15.0 is slated to land on v6.x on tuesday. Should we back it out or float this patch?

@refack
Copy link
Contributor

refack commented Nov 2, 2017

libuv 1.15.0 is slated to land on v6.x on tuesday. Should we back it out or float this patch?

IMHO floating the patch is the better path.

/cc @saghul

@MylesBorins
Copy link
Contributor

@refack my only concern is that it is practically Friday and the release is going out Tuesday.

We'll need to get that patch landed upstream ASAP, and even then I'd like to get a couple opinions about fast tracking it from @nodejs/lts

@gireeshpunathil
Copy link
Member

@MylesBorins: sorry - but can you please clarify which PR?

Should we back it out

@gibfahn
Copy link
Member

gibfahn commented Nov 3, 2017

@gireeshpunathil pretty sure the patch is libuv/libuv#1611

@gireeshpunathil
Copy link
Member

sorry for my ignorance but that is not yet landed (to be backed out) right?

@gibfahn
Copy link
Member

gibfahn commented Nov 3, 2017

Reopening because the issue isn't fixed till this patch lands in v8.x and v6.x.

libuv 1.15.0 is slated to land on v6.x on tuesday. Should we back it out or float this patch?

sorry for my ignorance but that is not yet landed (to be backed out) right?

@MylesBorins meant either back out libuv 1.15.0, or float this patch

Not sure if possible, but if libuv could include that patch in a 1.15.1, that would be better than than floating a patch.

I'd rather we float the patch than revert libuv 1.15.0 if possible.

MylesBorins pushed a commit to MylesBorins/node that referenced this issue Nov 3, 2017
Original commit message:
    tty, win: get SetWinEventHook pointer at startup

    SetWinEventHook is not available on some Windows versions.

    Fixes: nodejs#16603
    Reviewed-By: Ben Noordhuis <[email protected]>
    Reviewed-By: Colin Ihrig <[email protected]>

fixes: nodejs#16603
@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 3, 2017

cherry-pick in #16724

MylesBorins pushed a commit that referenced this issue Nov 4, 2017
Original commit message:
    tty, win: get SetWinEventHook pointer at startup

    SetWinEventHook is not available on some Windows versions.

    Fixes: #16603
    Reviewed-By: Ben Noordhuis <[email protected]>
    Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: #16724
Fixes: #16603
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 4, 2017
Original commit message:
    tty, win: get SetWinEventHook pointer at startup

    SetWinEventHook is not available on some Windows versions.

    Fixes: #16603
    Reviewed-By: Ben Noordhuis <[email protected]>
    Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: #16724
Fixes: #16603
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 4, 2017
Original commit message:
    tty, win: get SetWinEventHook pointer at startup

    SetWinEventHook is not available on some Windows versions.

    Fixes: #16603
    Reviewed-By: Ben Noordhuis <[email protected]>
    Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: #16724
Fixes: #16603
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@refack
Copy link
Contributor

refack commented Nov 5, 2017

Good job everyone 🥇 7 days from report to fix, including a roundtrip through libuv.

@StefanScherer
Copy link
Author

I want to thank you all involved resolving this so quickly. Can‘t wait for the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
windows Issues and PRs related to the Windows platform.
Projects
None yet
10 participants