-
Notifications
You must be signed in to change notification settings - Fork 3.9k
INTERNAL: Panic! This is a bug! at io.grpc.stub.ClientCalls.toStatusRuntimeException caused by NullPointerException #9706
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
Comments
Can you upgrade to the latest version - 1.51.0 and see if it still happens? |
Yah I can do that, but I wonder if there is any reason in particular we think that 1.51.0 would do better? |
Without looking at the details of the issue there is a small chance that the bug might have been fixed. Even otherwise 1.37 is 18 months old so we always recommend upgrading to the latest version. |
Looks like the Metadata.Marshaller returned null. That is a bug. We could have some code to detect this case earlier and make it a more clear exception. But the actual fix is almost certainly in the application. |
Actually, I do see some weird case maybe when metadata.remove() is used. Dunno. But this isn't something we've seen before and it isn't worth our time to debug the year-and-a-half-old 1.37.0, which won't even get updates if we find a problem. I still expect this to happen on newer versions and Metadata hasn't changed much, but retries have had plenty of fixes. |
I can reproduce this on master, but only if the Marshaller is returning diff --git a/api/src/test/java/io/grpc/MetadataTest.java b/api/src/test/java/io/grpc/MetadataTest.java
index 51aecc638..594e2520b 100644
--- a/api/src/test/java/io/grpc/MetadataTest.java
+++ b/api/src/test/java/io/grpc/MetadataTest.java
@@ -119,6 +119,26 @@ public class MetadataTest {
private static final Metadata.Key<Fish> KEY_IMMUTABLE =
Metadata.Key.of("immutable-bin", IMMUTABLE_FISH_MARSHALLER);
+ @Test
+ public void foo() {
+ Metadata metadata = new Metadata();
+ Metadata.Key<byte[]> key = Metadata.Key.of("foo-bin", new Metadata.BinaryMarshaller<byte[]>() {
+ @Override
+ public byte[] toBytes(byte[] value) {
+ return null;
+ }
+
+ @Override
+ public byte[] parseBytes(byte[] serialized) {
+ return serialized;
+ }
+ });
+ metadata.put(key, new byte[0] /* value ignored */);
+ // null is not allowed. It throws NPE during put()
+ //metadata.put(key, null);
+ metadata.serialize();
+ }
+
@Test
public void noPseudoHeaders() {
thrown.expect(IllegalArgumentException.class); |
I see, so the application error must occur when the marshaller in attempts to get the bytes returns null? We're using the ascii string marshaller, is it possible there is an error there? Looking at the code, it mentions that it only works with characters listed from a specific class. Do you know what class it is talking about or which comment? |
I don't think that error is possible with an AsciiMarshaller. It was looking like it needed to be a BinaryMarshaller. So either there is another way to trigger it or one of the very BinaryMarshallers is getting something wrong. |
The other idea I had was I saw that Metadata.java is not threadsafe, but I thought grpc clients were usable in concurrent use cases? |
You can do lots of concurrent RPCs, but you shouldn't share Metadata across them. The stubs create a new metadata object for each RPC. The main thing that prevents concurrent access of Metadata is only using it when in full control of it. The moment you pass Metadata to the next interceptor, for example, the code gives up the right to access the Metadata. |
#9781 is merged which would detect broken Marshallers early. It wouldn't detect an issue if it was due to application multithreading. |
It doesn't seem we can make any more progress on this, as we still suspect something in the application may be causing the issue. The fix for early NPE checking will be available in 1.53, which may help, but that seems to be all we can do at the moment. If you find out more, either nowish or once upgrading to 1.53, comment and we can reopen. You can also open a new issue if this gets locked before you get to that point. |
Hey so I figured out what the cause of the issue was: We were modifying the headers in a non-thread safe way. Specifically the code I posted omitted that we were removing the headers on each call which was happening concurrently. Something like:
I really appreciate the pointers and the change made. |
Thanks for closing the loop and confirming the theory. I'm glad the earlier error checking made it easier to track down. |
What version of gRPC-Java are you using?
1.37.0
What is your environment?
jdk 1.8, appengine
What did you expect to see?
Expect to see a successful call, I have also added retry on Internal and that does not seem to work for this error. Though I've tested it works with typical INTERNAL errors.
What did you see instead?
Internal Runtime Exception, the request does not make it out from the client.
Steps to reproduce the bug
I am unable to reproduce the bug, it sporadically occurs.
Here is how I setup my stub:
And here is my retry_config:
The text was updated successfully, but these errors were encountered: