InProcessChannel with optional stack trace

[bojanv55]

What version of gRPC are you using?

1.19.0

What did you expect to see?

Error Stack trace.

In reference to this stack overflow question: here

Is it possible to have an option to show full stack trace for InProcessChannel, since I use this in integration tests and I need to see full reason of exception?

[ejona86]

An option here is to add causal information to the Status, with the original Exception. That would happen here:

grpc-java/core/src/main/java/io/grpc/internal/ServerImpl.java

Lines 761 to 763 in e2bb441

	private void internalClose() {
	// TODO(ejona86): this is not thread-safe :)
	stream.close(Status.UNKNOWN, new Metadata());

We would then need an option in in-process to also copy cause here:

grpc-java/core/src/main/java/io/grpc/inprocess/InProcessTransport.java

Lines 845 to 859 in e2bb441

	/**
	* Returns a new status with the same code and description, but stripped of any other information
	* (i.e. cause).
	*
	* <p>This is, so that the InProcess transport behaves in the same way as the other transports,
	* when exchanging statuses between client and server and vice versa.
	*/
	private static Status stripCause(Status status) {
	if (status == null) {
	return null;
	}
	return Status
	.fromCodeValue(status.getCode().value())
	.withDescription(status.getDescription());
	}

It's probably best to still create a new Status in in-process, just so that no pesky status1 == status2 code crops up, which should be using equals() instead.

[reggiemcdonald]

Im a computer science student thats new to open source - could I give this one a try?

[ejona86]

Go for it.

[reggiemcdonald]

Following your suggestion, I added the cause to the status in internalClose:

private void internalClose(Throwable t) {
    stream.close(Status.UNKNOWN.withCause(t), new Metadata());
}

and I added the ability to copy status over in InProcessTransport:

private static Status stripCause(Status status, boolean includeCauseWithStatus) {
    if (status == null) {
      return null;
    }
    if (includeCauseWithStatus) {
      return Status
          .fromCodeValue(status.getCode().value())
          .withCause(status.getCause())
          .withDescription(status.getDescription());
    }
    return Status
        .fromCodeValue(status.getCode().value())
        .withDescription(status.getDescription());
  }

But all the unit tests assume that ServerImpl does not pass cause back. Would you recommend refactoring the unit tests, or approaching this in a different way?

[ejona86]

includeCauseWithStatus should default to false, the current behavior. Then you can add a single new unit test that configures the value to true and verify that the original exception is propagated. I don't see an ideal example to copy, but one of the failing tests may prove to be one. Could you mention some that are failing?

Nit: you may consider this code flow:

Status status = Status
    .fromCodeValue(status.getCode().value())
    .withDescription(status.getDescription());
if (includeCauseWithStatus) {
  status = status.withCause(status.getCause());
}
return status;

[reggiemcdonald]

Thats much cleaner syntax! Will revise.

I do set the default of includeCauseWithStatus to false. However, there are six unit tests failing. They are all failing for the same reason. Here is an example, and an explanation:

grpc-java/core/src/test/java/io/grpc/internal/ServerImplTest.java

Lines 1188 to 1211 in ee8b395

	public void messageRead_errorCancelsCall() throws Exception {
	JumpToApplicationThreadServerStreamListener listener
	= new JumpToApplicationThreadServerStreamListener(
	executor.getScheduledExecutorService(),
	executor.getScheduledExecutorService(),
	stream,
	Context.ROOT.withCancellation(),
	PerfMark.createTag());
	ServerStreamListener mockListener = mock(ServerStreamListener.class);
	listener.setListener(mockListener);
	TestError expectedT = new TestError();
	doThrow(expectedT).when(mockListener)
	.messagesAvailable(any(StreamListener.MessageProducer.class));
	// Closing the InputStream is done by the delegated listener (generally ServerCallImpl)
	listener.messagesAvailable(mock(StreamListener.MessageProducer.class));
	try {
	executor.runDueTasks();
	fail("Expected exception");
	} catch (TestError t) {
	assertSame(expectedT, t);
	ensureServerStateNotLeaked();
	}
	}

In the catch block there is a call to ensureServerStateNotLeaked(). It uses an argumentCaptor to assert that the call to stream.close in ServerImpl.internalClose is equal to Status.UNKNOWN. Furthermore, it asserts that the status has a null cause

grpc-java/core/src/test/java/io/grpc/internal/ServerImplTest.java

Lines 1441 to 1446 in ee8b395

	private void ensureServerStateNotLeaked() {
	verify(stream).close(statusCaptor.capture(), metadataCaptor.capture());
	assertEquals(Status.UNKNOWN, statusCaptor.getValue());
	assertNull(statusCaptor.getValue().getCause());
	assertTrue(metadataCaptor.getValue().keys().isEmpty());
	}

[ejona86]

Looks like that code originates from 2b1eee9 . Since the cause is not actually transmitted, it seems safe to remove the assertNull(statusCaptor.getValue().getCause()); check. We might want to actually change at least one of those tests to verify that the cause is set (with a comment saying it is used by in-process, and potentially debugging).