grpc-dart icon indicating copy to clipboard operation
grpc-dart copied to clipboard

lib/src/server/handlers.dart 's _customHeaders is reset to null, it should be reset to empty Map instead

Open isoos opened this issue 1 year ago • 7 comments

Error report: https://www.reddit.com/r/dartlang/comments/1fk84bb/unhandled_exception_from_grpc_package/

The reported line seems to be when forcing _customHeaders to not-null: https://github.com/grpc/grpc-dart/blob/master/lib/src/server/handler.dart#L384

Looking at the git blame history, I think _customHeaders = null should have been migrated to _customHeaders = <String, String>{} when the null-safety migration happened.

isoos avatar Sep 19 '24 06:09 isoos

I think the current code is correct. You can only send headers once so they are intentionally reset to null after that. You should not be able to enter this code again (it is additionally guarded by checking _headersSent).

I also see that the benchmark code in question is referencing Dart 2.19 and pubspec.lock mentions grpc version 3.1. This is really old stuff.

We will need to know exact stack trace and exact version of grpc package for this to be actionable.

For now I am going to close this - but if r/David_Owens provides necessary info I can reopen.

mraleph avatar Sep 19 '24 11:09 mraleph

Here is a screen shot of the stack trace. It's running in a Docker container and only comes up for a split second, so I had to take a video of it.

https://1drv.ms/i/c/29ea63e9e7ad0202/EQ3bEw8Cer1KrEQHk-vpYmABnqFhUoNf85uwvdy7C0MLqQ?e=oGMD3A

0 ServerHandler.sendTrailers (package:grpc/src/server/handler.dart:384) 1 ServerHandler._sendError (package:grpc/src/server/handler.dart:459) 2 ServerHandler._onResponse (package:grpc/src/server/handler.dart:327) ...

owensdj avatar Sep 19 '24 11:09 owensdj

@owensdj what is the version of the package / Dart SDK you are using?

mraleph avatar Sep 19 '24 11:09 mraleph

gRPC 4.0.1 and Dart SDK 3.5.2. The problem happened with older versions as well.

owensdj avatar Sep 19 '24 11:09 owensdj

One possible issue is that here https://github.com/grpc/grpc-dart/blob/master/lib/src/server/handler.dart#L360-L367 _stream.sendHeaders can trigger an error at which point _customHeaders will end up null but _headersSent is not true. This might be an underlying root cause.

mraleph avatar Sep 19 '24 11:09 mraleph

@mraleph That makes sense, but if you look at the call stack I don't see where the execution would have set _customHeaders to null. The only places where it's ever set to null is in sendHeaders on line 361 and in sendTrailers on line 388. I don't see sendHeaders in the call stack and it crashes at line 384 in sendTrailers before it gets to the line setting _customHeaders to null.

owensdj avatar Sep 19 '24 20:09 owensdj

That is correct that we don't see sendHeaders in the stack trace, but we see _sendError and a bunch of async machinery. It indicates that some sort of error happened and we are trying to propagate it. Also the explanation which I gave is based on the logic - that's the only possible sequence of events which leads to this.

If I put throw 'aaa'; right before sendHeaders then this causes exactly the type of a crash that you have reported:

Unhandled exception:
Null check operator used on a null value
#0      ServerHandler.sendTrailers (package:grpc/src/server/handler.dart:385:21)
#1      ServerHandler._sendError (package:grpc/src/server/handler.dart:460:5)
#2      ServerHandler._onResponse (package:grpc/src/server/handler.dart:327:7)
#3      _RootZone.runUnaryGuarded (dart:async/zone.dart:1609:10)
#4      _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart:366:11)
#5      _BufferingStreamSubscription._add (dart:async/stream_impl.dart:297:7)
#6      _SyncStreamControllerDispatch._sendData (dart:async/stream_controller.dart:777:19)
#7      _StreamController._add (dart:async/stream_controller.dart:651:7)
#8      new Stream.fromFuture.<anonymous closure> (dart:async/stream.dart:249:18)
#9      Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:902:45)
#10     Future._propagateToListeners (dart:async/future_impl.dart:931:13)
#11     Future._chainCoreFutureSync (dart:async/future_impl.dart:632:7)
#12     Future._chainCoreFutureAsync.<anonymous closure> (dart:async/future_impl.dart:677:7)
#13     _microtaskLoop (dart:async/schedule_microtask.dart:40:21)
#14     _startMicrotaskLoop (dart:async/schedule_microtask.dart:49:5)
#15     _runPendingImmediateCallback (dart:isolate-patch/isolate_patch.dart:118:13)
#16     _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:185:5)

So I think this is the most possible explanation.

mraleph avatar Sep 20 '24 13:09 mraleph

Hello 👋 Is there any possible fix for this issue? We are observing this in production as well.

@mraleph https://github.com/grpc/grpc-dart/pull/801 can we merge something like this?

hiro-at-pieces avatar Sep 01 '25 20:09 hiro-at-pieces