Skip to content

Conversation

@oleg-nenashev
Copy link
Member

Follow-up to #49 from @stephanreiter

When I was testing the change on my laptop before the release (Windows 10 64bit latest on MacBook Pro 2014), I have got the flaky test issue. So I have improved diagnostics and error propagation to understand what's going on:

Example of the new error:

testSendCtrlC_nonExistentPID(org.jvnet.winp.NativeAPITest)  Time elapsed: 0.001 sec  <<< ERROR!
org.jvnet.winp.WinpException: External Ctrl+C execution failed for process pid=2147483647. Ctrl+C process exited with code 87: Failed to attach to the console (see the AttachConsole WinAPI call). error=0 at winp.cpp:59
        at org.jvnet.winp.Native.sendCtrlC(Native Method)
        at org.jvnet.winp.Native.sendCtrlC(Native.java:89)
        at org.jvnet.winp.WinProcess.sendCtrlC(WinProcess.java:93)
        at org.jvnet.winp.NativeAPITest.testSendCtrlC_nonExistentPID(NativeAPITest.java:163)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
        at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
        at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
        at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
        at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
        at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
        at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
        at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
        at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
        at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
        at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
        at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
        at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
        at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)

Due to whatever reason the process sometimes does not exist by the time the console attached to it. It does not seem to be a process completion, because the timeout didn't happen so far. The issue also happens only during build.cmd cleanbuild, but not during Unit test runs from IDE. After raising the timeouts and adding diagnostics the issue happens almost every time.

@stephanreiter I would appreciate your feedback. I still can release alpha with a broken test if needed, but I want to have this diagnostics in place at least

@stephanreiter
Copy link

I'll try to find out what's going on. Thanks for improving the diagnostics and commenting code! That's very good.

@stephanreiter
Copy link

I think the PID that is sent to the CTRL+C sending exe is invalid. Very suspicous 2147483647=0x7FFFFFFF.

@oleg-nenashev
Copy link
Member Author

@stephanreiter That one is fine, because it comes from the new testSendCtrlC_nonExistentPID test. I can check what the existing test returns (whether the PID is correct). There is a possibility that there are type conversion errors somewhere, need to carefully review the code

@stephanreiter
Copy link

Ok! I am trying to reproduce it on a Win10 64-bit but couldn't so far.

@stephanreiter
Copy link

Could you take a look at https://github.com/stephanreiter/winp/commit/c02d6a2ac0d698169d031610352249e32cb5dbf9
I am using jlong for HANDLE, maybe a process handle is truncated on your system (due to extra activity in the cleanbuild).
Also, from WinXP SP1 on we can use GetProcessId. Maybe worth considering?

@oleg-nenashev
Copy link
Member Author

@stephanreiter Yes, sounds plausible.
I will merge this code if you do not mind.

Regarding https://github.com/stephanreiter/winp/commit/c02d6a2ac0d698169d031610352249e32cb5dbf9 , my proposal would be to create a separate pull request. Maybe we should change the return value of JNIEXPORT jint JNICALL Java_org_jvnet_winp_Native_getProcessId to jlong and create a new method in Java API for long PIDs.

@stephanreiter
Copy link

Go ahead and merge it. I'll take care of a separate pull request regarding getProcessId. :-)

@oleg-nenashev oleg-nenashev merged commit a95c8db into jenkinsci:master Jun 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants