Do not print exception stack trace through System.err

[dani8art]

This library makes use of ex.printStackTrace() at some point in its code where propagation of an exception is not possible, there is not surface where to place the exception or it can't be consumed. We understand sometimes it is needed and it helps debugging but this method uses System.err as default PrintWriter which is causing issues because dependant apps can't mute these lines in their logs.

The three main points where ex.printStackTrace() is used are the following

• /util/src/main/java/io/kubernetes/client/Exec.java https://github.com/kubernetes-client/java/blob/62d6c626ec1a2fd72f2da7e17a822f405b8445ac/util/src/main/java/io/kubernetes/client/Exec.java#L521
• /extended/src/main/java/io/kubernetes/client/extended/kubectl/KubectlExec.java https://github.com/kubernetes-client/java/blob/f20788272291c0e79a8c831d8d5a7dd94d96d2de/extended/src/main/java/io/kubernetes/client/extended/kubectl/KubectlExec.java#L76
• /extended/src/main/java/io/kubernetes/client/extended/kubectl/KubectlPortForward.java https://github.com/kubernetes-client/java/blob/f20788272291c0e79a8c831d8d5a7dd94d96d2de/extended/src/main/java/io/kubernetes/client/extended/kubectl/KubectlPortForward.java#L107

Replacement proposal

If we use slf4j api, we will be able to handle the visibility of these lines from outside the library. That way dependant apps may eventually hide these traces from their logs. We have made some tests and it is totally safe to replace ex.printStackTrace() by the following

  @Test
  public void stacktrace() {
    try {
      throw new RuntimeException("error");
    } catch (RuntimeException e) {
      log.debug(e.getMessage(), e);
    }
  }

Resulting in the following lines that in this case can be hidden if we use a proper configuration in our apps.

10:08:10.355 [main] DEBUG  i.k.client.util.StackTraceTest - java.lang.RuntimeException: error
java.lang.RuntimeException: error
	at io.kubernetes.client.util.StackTraceTest.stacktrace(StackTraceTest.java:19)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
...

It would be very helpful to us if the library handle this log lines using slf4j, does this proposal make sense for you?

[brendandburns]

I never liked the current approach (as my comment suggests :)) but I think if we're going to refactor this, let's make it take in an optional "unhandledExceptionHandler" or somesuch and pass it to that. I think that gives more flexibility to the individual user.

[dani8art]

Hi @brendandburns, thanks for answering!

do you mean to replace it with the following?

@Override
public void failure(Throwable ex) {
  super.failure(ex);
  unhandledExceptionHandler.accept(ex);

  synchronized (ExecProcess.this) {
    // Try for a pretty unique error code, so if someone searches
    // they'll find this
    // code.
    statusCode = -1975219;
    isAlive = false;
    ExecProcess.this.latch.countDown();
  }
}

If so, I don't see how this could help, I mean, the web socket stream has failed and then the ExecProcess must be finished returning an error statusCode as it is performed right now, and the user could use this statusCode to handle the error. The exception at this moment only adds debug info. When a user is running it the only thing that would do is process the message because they can't do more for the connection, right? Something like

ExecProcess process = new ExecProcess(apiClient, ex -> {
  log.debug(ex.getMessage(), ex);
});

process.wait();

In this case, I just only add a new property to ExecProcess where to store this info like the following.

@Override
public void failure(Throwable ex) {
  super.failure(ex);
  unhandledException = ex;
  ...

So maybe the user could handle it like

ExecProcess process = new ExecProcess(apiClient);

process.wait();

if (process.exitValue() == 0) {
  // succeeded command
} else if (process.exitValue() == -1975219) {
  throw new RuntimeException("ExecProcess unhandled exception", process.unhandledException());
} else {
  // failed command
}

Did I understand you correctly? Could you add more info about your proposal, please?

Thanks, BR

[brendandburns]

Here's what I was thinking:

Exec ex = new Exec();
ex.setExceptionHandler(...);
...
Kubectl.exec().exceptionHandler(...).execute();

If the user doesn't set the exception handler, it prints to stderr. If the user does set the exception handler, the exception is passed to the handler which can do whatever it wants.

That feels more general purpose to me if we're going to be improving this code.

[dani8art]

Ok that sounds reasonable, let's move the conversation to the PR https://github.com/kubernetes-client/java/pull/2366

Thanks!

[dani8art]

Hi @brendandburns, great to see it has been merged into master, would you have an estimation on when it could be released?

Thanks!

[brendandburns]

~I'll cut a release today.~

Apologies, I need you to create a cherry-pick PR into the release-16 branch before we can cut a 16.0.1 release branch.

[dani8art]

Ok, that's fine, I'll subscribe to the releases feed so I can't get notified when it happens. Thanks so much!

[dani8art]

Hi @brendandburns, any news on this? It would be great to have this released. Could I help with something? Thanks!

[brendandburns]

Closing this since it is in master and the cherry-pick was released.