grpc / grpc-dart

The Dart language implementation of gRPC.
https://pub.dev/packages/grpc
Apache License 2.0
852 stars 267 forks source link

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

Open isoos opened 1 week ago

isoos commented 1 week ago

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.

mraleph commented 1 week ago

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.

owensdj commented 1 week ago

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) ...

mraleph commented 1 week ago

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

owensdj commented 1 week ago

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

mraleph commented 1 week ago

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.

owensdj commented 1 week ago

@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.

mraleph commented 1 week ago

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.