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

core: rollback executor supplier, needs investigation #8289

Merged
merged 2 commits into from
Jun 25, 2021

Conversation

YifeiZhuang
Copy link
Contributor

No description provided.

@YifeiZhuang YifeiZhuang requested a review from ejona86 June 25, 2021 19:49
@ejona86
Copy link
Member

ejona86 commented Jun 25, 2021

We saw a test failure with:

Exception while executing runnable io.grpc.internal.ServerImpl$JumpToApplicationThreadServerStreamListener$1Closed@123feec
java.lang.IllegalStateException: listener unset
	at io.grpc.internal.ServerImpl$JumpToApplicationThreadServerStreamListener.getListener(ServerImpl.java:796)
	at io.grpc.internal.ServerImpl$JumpToApplicationThreadServerStreamListener.access$3000(ServerImpl.java:773)
	at io.grpc.internal.ServerImpl$JumpToApplicationThreadServerStreamListener$1Closed.runInContext(ServerImpl.java:911)
	at io.grpc.internal.ContextRunnable.run(ContextRunnable.java:37)
	at io.grpc.internal.SerializeReentrantCallsDirectExecutor.completeQueuedTasks(SerializeReentrantCallsDirectExecutor.java:67)
	at io.grpc.internal.SerializeReentrantCallsDirectExecutor.execute(SerializeReentrantCallsDirectExecutor.java:54)
	at io.grpc.internal.ServerImpl$ServerTransportListenerImpl.streamCreatedInternal(ServerImpl.java:641)
	at io.grpc.internal.ServerImpl$ServerTransportListenerImpl.streamCreated(ServerImpl.java:465)
	at io.grpc.inprocess.InProcessTransport$InProcessStream$InProcessClientStream.start(InProcessTransport.java:817)
	at io.grpc.internal.ForwardingClientStream.start(ForwardingClientStream.java:92)
	at io.grpc.internal.InternalSubchannel$CallTracingTransport$1.start(InternalSubchannel.java:681)
	at io.grpc.internal.ClientCallImpl.startInternal(ClientCallImpl.java:283)
	at io.grpc.internal.ClientCallImpl.start(ClientCallImpl.java:188)
	at io.grpc.census.CensusTracingModule$TracingClientInterceptor$1.start(CensusTracingModule.java:393)
	at io.grpc.census.CensusStatsModule$StatsClientInterceptor$1.start(CensusStatsModule.java:696)
	at io.grpc.stub.ClientCalls.startCall(ClientCalls.java:332)
	at io.grpc.stub.ClientCalls.asyncUnaryRequestCall(ClientCalls.java:306)
	at io.grpc.stub.ClientCalls.futureUnaryCall(ClientCalls.java:218)

@YifeiZhuang YifeiZhuang merged commit 1b57d48 into grpc:master Jun 25, 2021
@YifeiZhuang
Copy link
Contributor Author

io.grpc.internal.ServerImpl$JumpToApplicationThreadServerStreamListener$1Closed@123feec
java.lang.IllegalStateException: listener unset

This log line might be caused by closing the stream but not set NoopListener yet.
SerializeReentrantCallsDirectExecutor running with InProcessTransport triggers this when everything runs inline, handleServerCall() not yet scheduled.

It might be causing TAP failures java.lang.AssertionError: Unexpected error logged. , but not sure others https://test.corp.google.com/ui#cl=381473612.

@ejona86
Copy link
Member

ejona86 commented Jun 28, 2021

Oh, directExecutor is causing work to be scheduled before the HandleServerCall Runnable is scheduled. Seems that requires triggering the UNIMPLEMENTED flow that calls stream.close(), since the application does not yet have access to the stream/call. And yeah, that could probably happen without directExecutor because of #7921 .

We'll need to re-evaluate our approach. We could just set the Listener in the UNIMPLEMENTED flow before cancelling the stream. We could instead add a SerializingExecutor.enqueue() that just calls runQueue.add(). We could do the same for SerializeReentrantCallsDirectExecutor, but it currently lazily initializes its queue. I have a suspicion that the taskQueue is generally allocated, so it may not hurt anything. And most alternatives seems to involve an allocation of their own.

YifeiZhuang added a commit that referenced this pull request Jun 28, 2021
* Revert "core: rollback executor supplier, needs investigation (#8289)"

This reverts commit 1b57d48.

* set jumpStream listener before close stream
YifeiZhuang added a commit to YifeiZhuang/grpc-java that referenced this pull request Jul 25, 2021
* Revert "core: fix boq conformance failures (grpc#8281)"

This reverts commit c1ad5de.

* Revert "core: allow per-service/method executor (grpc#8266)"

This reverts commit c540229.
YifeiZhuang added a commit to YifeiZhuang/grpc-java that referenced this pull request Jul 25, 2021
…c#8290)

* Revert "core: rollback executor supplier, needs investigation (grpc#8289)"

This reverts commit 1b57d48.

* set jumpStream listener before close stream
@YifeiZhuang YifeiZhuang deleted the zivy/executor_rollback branch September 9, 2021 01:35
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants