Handle the case when `watch.exitCode` may return null #1366

kuzaxak · 2023-04-26T17:08:19Z

After changes introduced in #1260 we have frequent error with NullPointerException. Based on a documentation for watch.exitCode method it's clear that watch.exitCode() can return null if close is received before the exit code. In such a case, you can handle the null value and provide an appropriate default or error value.

java.lang.NullPointerException at
org.csanchez.jenkins.plugins.kubernetes.pipeline.ContainerExecProc.join(ContainerExecProc.java:89)
at hudson.Proc.joinWithTimeout(Proc.java:174)
at com.cloudbees.jenkins.plugins.sshagent.exec.ExecRemoteAgent.stop(ExecRemoteAgent.java:129)

Fix for JENKINS-71135

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

jglick · 2023-04-26T18:26:32Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecProc.java

+
+            CompletableFuture<Integer> exitCodeFuture = watch.exitCode();
+            if (exitCodeFuture == null) {
+                LOGGER.log(Level.FINEST, "Watcher return 'null' instead of exitCode");


Maybe we should extend

kubernetes-plugin/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecDecorator.java

Lines 563 to 567 in 294ad42

doExec(in, !launcher.isUnix(), printStream, masks, commands);

LOGGER.log(Level.INFO, "Created process inside pod: [" + getPodName() + "], container: ["

+ containerName + "]" + "[" + TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startMethod) + " ms]");

ContainerExecProc proc = new ContainerExecProc(watch, alive, finished, stdin);

to pass the printStream to proc so unusual issues can be printed in the build log, rather than the system log where they would only be visible to admins.

Good idea, but I don't know how to do it in a better way. Can I add an additional arg for ContainerExecProc to pass printStream and then use it to print a log?

I think it can be implemented as a separate PR to unblock the issue fix. I can prepare a PR for that.

Can I add an additional arg

That is what I was thinking.

I think it can be implemented as a separate PR

Agreed, it was just an idea.

annaagaf · 2023-05-05T08:25:56Z

We have been running this build and it's still throwing NPEs at exitCodeFuture.join() :

10:31:47  java.lang.NullPointerException
10:31:47  	at org.csanchez.jenkins.plugins.kubernetes.pipeline.ContainerExecProc.join(ContainerExecProc.java:97)
10:31:47  	at hudson.Proc.joinWithTimeout(Proc.java:174)
10:31:47  	at com.cloudbees.jenkins.plugins.sshagent.exec.ExecRemoteAgent.stop(ExecRemoteAgent.java:129)

annaagaf · 2023-05-11T06:49:58Z

@jglick Added a commit to address another NullPointerException issue. We will also be testing this build on our Jenkins.

annaagaf · 2023-05-11T07:53:54Z

The updated version is still throwing NPEs, back to the drawing board.

10:21:32  java.lang.NullPointerException
10:21:32  	at org.csanchez.jenkins.plugins.kubernetes.pipeline.ContainerExecProc.join(ContainerExecProc.java:99)
10:21:32  	at hudson.Proc.joinWithTimeout(Proc.java:174)

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecProc.java

annaagaf · 2023-05-24T08:42:29Z

Updated this PR to the current state we are running. This solves NPEs and improves logging; however, this now shows the original issue, which is with ssh-agent plugin (ssh-agent -k failing at the end of pipeline). Will be patching that plugin to validate.

Addressed, I guess (but please refrain from force-pushing since it makes it much harder for reviewers to track what you changed and when)

Vlatombe · 2023-05-26T08:29:31Z

pom.xml

@@ -3,7 +3,7 @@
  <parent>
    <groupId>org.jenkins-ci.plugins</groupId>
    <artifactId>plugin</artifactId>
-    <version>4.61</version>
+    <version>4.62</version>


Please, for next time, avoid introducing unrelated changes, as it makes the review work harder and increases the likelihood of merge conflicts.

Version bumps like this are usually handled via automated PRs filed by Dependabot, e.g. #1382

Thanks for the note! Currently rebased to latest upstream and this bump is no longer included with PR.

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecProc.java

mbecca · 2023-09-01T15:52:32Z

Hello, does anyone have a date to include this bug fix?

After changes introduced in [PR][1] we have frequent error with `NullPointerException`. Based on a [documentation][2] for `watch.exitCode` method it's clear that watch.exitCode() can return null if close is received before the exit code. In such a case, you can handle the null value and provide an appropriate default or error value. ``` java.lang.NullPointerException at org.csanchez.jenkins.plugins.kubernetes.pipeline.ContainerExecProc.join(ContainerExecProc.java:89) at hudson.Proc.joinWithTimeout(Proc.java:174) at com.cloudbees.jenkins.plugins.sshagent.exec.ExecRemoteAgent.stop(ExecRemoteAgent.java:129) ``` Fix for JENKINS-71135 [1]: #1260 [2]: https://github.com/fabric8io/kubernetes-client/blob/8a879128d893627a40d64cf2a9c7d5da2e7f47d2/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/dsl/ExecWatch.java#L68-L82

Completable future can be completed exceptionally with NullPointerException, in which case join() would re-throw this exception. Replacing join() with get(), which throws ExecutionException instead, which wraps the original exception. This exception is caught and handled appropriately.

Adding logging of errors in build log for improved visibility.

…/ContainerExecProc.java Co-authored-by: Vincent Latombe <vincent@latombe.net>

anna-agafonova · 2023-09-06T08:25:41Z

@Vlatombe @jglick addressed the PR comments, please re-review

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecProc.java

jglick · 2023-09-06T22:21:07Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecProc.java

-            return watch.exitCode().join();
+
+            CompletableFuture<Integer> exitCodeFuture = watch.exitCode();
+            Integer exitCode = exitCodeFuture.get();


Does this not contradict the PR title, that it is possible exitCodeFuture == null? Either the code or the PR title is incorrect.

(Docs are ambiguous whether null refers to CompletableFuture<Integer>, or the value of the CompletableFuture.)

The goal here is to handle all possible outcomes (null exitCode is handled in this block, null exitCodeFuture in catchall block, checked ExecutionException that can be thrown by get() in catch block).
The PR title refers to the original issue that's causing NPEs on current master when exitCode is null, but what would you suggest as a better naming/solution here?

null exitCodeFuture in catchall block

If you mean in the catch (Exception block, then yes, but code should never knowingly be catching NullPointerException; it should check for null. Conversely, if you were relying on arbitrary unknown exceptions, you might as well keep it simple

try { return watch.exitCode().join(); } catch (Exception x) { LOGGER.log(Level.FINEST, "Exception occurred while waiting for exit code", x); return -1; }

Alright, agreed - how about now?

@jglick could you please re-review?

…/ContainerExecProc.java Co-authored-by: Jesse Glick <jglick@cloudbees.com>

jglick · 2023-09-14T18:50:11Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecProc.java

-            return watch.exitCode().join();
+
+            CompletableFuture<Integer> exitCodeFuture = watch.exitCode();
+            Integer exitCode = exitCodeFuture.get();


null exitCodeFuture in catchall block

If you mean in the catch (Exception block, then yes, but code should never knowingly be catching NullPointerException; it should check for null. Conversely, if you were relying on arbitrary unknown exceptions, you might as well keep it simple

try { return watch.exitCode().join(); } catch (Exception x) { LOGGER.log(Level.FINEST, "Exception occurred while waiting for exit code", x); return -1; }

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecProc.java

…/ContainerExecProc.java Co-authored-by: Jesse Glick <jglick@cloudbees.com>

kuzaxak requested a review from a team as a code owner April 26, 2023 17:08

jglick approved these changes Apr 26, 2023

View reviewed changes

jglick previously requested changes May 11, 2023

View reviewed changes

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecProc.java Outdated Show resolved Hide resolved

Vlatombe reviewed May 26, 2023

View reviewed changes

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecProc.java Outdated Show resolved Hide resolved

Vlatombe reviewed May 26, 2023

View reviewed changes

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/ContainerExecProc.java Outdated Show resolved Hide resolved

Vlatombe added the bug Bug Fixes label May 26, 2023

kuzaxak and others added 9 commits September 6, 2023 09:30

Add error logging in build log

c274e2e

Adding logging of errors in build log for improved visibility.

Update src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline…

b17e476

…/ContainerExecProc.java Co-authored-by: Vincent Latombe <vincent@latombe.net>

Update src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline…

88bcfbe

…/ContainerExecProc.java Co-authored-by: Vincent Latombe <vincent@latombe.net>

Improve log and printstream messages

639a013

Add missing import

6698b68

Reorder imports

468cef0

jglick reviewed Sep 6, 2023

View reviewed changes

Update src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline…

b82bd2b

…/ContainerExecProc.java Co-authored-by: Jesse Glick <jglick@cloudbees.com>

jglick reviewed Sep 14, 2023

View reviewed changes

anna-agafonova and others added 2 commits September 20, 2023 09:28

Update src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline…

f58762b

…/ContainerExecProc.java Co-authored-by: Jesse Glick <jglick@cloudbees.com>

Improve error handling to avoid generic catchall

0bc0aad

jglick requested a review from Vlatombe October 4, 2023 20:45

Vlatombe approved these changes Oct 10, 2023

View reviewed changes

Vlatombe merged commit 2da8e27 into jenkinsci:master Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle the case when `watch.exitCode` may return null #1366

Handle the case when `watch.exitCode` may return null #1366

kuzaxak commented Apr 26, 2023 •

edited by jglick

Loading

jglick Apr 26, 2023

kuzaxak Apr 27, 2023

kuzaxak Apr 27, 2023

jglick Apr 27, 2023

annaagaf commented May 5, 2023

annaagaf commented May 11, 2023

annaagaf commented May 11, 2023

annaagaf commented May 24, 2023

Vlatombe May 26, 2023 •

edited

Loading

anna-agafonova Sep 6, 2023

mbecca commented Sep 1, 2023

anna-agafonova commented Sep 6, 2023

jglick Sep 6, 2023

jglick Sep 6, 2023

anna-agafonova Sep 13, 2023

jglick Sep 14, 2023

anna-agafonova Sep 20, 2023

anna-agafonova Oct 4, 2023

jglick Sep 14, 2023

	doExec(in, !launcher.isUnix(), printStream, masks, commands);

	LOGGER.log(Level.INFO, "Created process inside pod: [" + getPodName() + "], container: ["
	+ containerName + "]" + "[" + TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startMethod) + " ms]");
	ContainerExecProc proc = new ContainerExecProc(watch, alive, finished, stdin);

Handle the case when watch.exitCode may return null #1366

Handle the case when watch.exitCode may return null #1366

Conversation

kuzaxak commented Apr 26, 2023 • edited by jglick Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annaagaf commented May 5, 2023

annaagaf commented May 11, 2023

annaagaf commented May 11, 2023

annaagaf commented May 24, 2023

Vlatombe May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbecca commented Sep 1, 2023

anna-agafonova commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Handle the case when `watch.exitCode` may return null #1366

Handle the case when `watch.exitCode` may return null #1366

kuzaxak commented Apr 26, 2023 •

edited by jglick

Loading

Vlatombe May 26, 2023 •

edited

Loading