[DDS] Two simultaneous 'futterVersion' requests to DDS fail with 'Null check operator used on a null value'

[annagrin]

Repro: PR: flutter/flutter#86832 (would need to undo the last commit where I disabled the test again)

07:41 +19 -1: test/web.shard/vm_service_web_test.dart: Clients of flutter run on web with DDS enabled can validate flutter version in parallel [E]
streamListen: (-32000) Null check operator used on a null value
package:vm_service/src/vm_service.dart 1626:45 new _OutstandingRequest
package:vm_service/src/vm_service.dart 2109:21 VmService._call
package:vm_service/src/vm_service.dart 2056:7 VmService.streamListen
test/web.shard/vm_service_web_test.dart 129:16 validateFlutterVersion
test/web.shard/vm_service_web_test.dart 62:9 main..
DDS version: dds: 2.0.2
DWDS version: dwds: 11.1.2
VMService version: 7.1.1

Failing tests:

https://ci.chromium.org/ui/p/flutter/builders/try/Mac%20web_tool_tests/7333/overview
https://ci.chromium.org/ui/p/flutter/builders/try/Linux%20web_tool_tests/7679/overview

Related: #45569

[annagrin]

@bkonyi I saw a similar error in one of the latest flakes in Flutter web tool tests:

Flutter issue: flutter/flutter#84012
https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20web_tool_tests/2947/overview

00:41 +0 -1: test/web.shard/debugger_stepping_web_test.dart: Web debugger can step over statements [E]                                                                                                 
  getStack: (-32603) getStack: NoSuchMethodError: The method 'toJson' was called on null.
  Receiver: null
  Tried calling: toJson()
  #0      Object.noSuchMethod (dart:core-patch/object_patch.dart:63:5)
  #1      VmServerConnection._delegateRequest (package:vm_service/src/vm_service.dart:1602:28)
  <asynchronous suspension>
  
  package:vm_service/src/vm_service.dart 1626:45        new _OutstandingRequest
  package:vm_service/src/vm_service.dart 2109:21        VmService._call
  package:vm_service/src/vm_service.dart 1913:61        VmService.getStack
  test/integration.shard/test_driver.dart 334:42        FlutterTestDriver.getTopStackFrame
  ===== asynchronous gap ===========================
  test/integration.shard/test_driver.dart 343:25        FlutterTestDriver.getSourceLocation
  ===== asynchronous gap ===========================
  test/web.shard/debugger_stepping_web_test.dart 40:39  main.<fn>

[aam]

another failure on flutter hhh https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket.appspot.com/8840551373789240880/+/u/flutter_test_tool_tests/stdout

[annagrin]

I looks like the second stack trace I mentioned is due to getStack from dwds returning null (which should only happen when the application is not paused), but I cannot repro it on my local machines.

[bkonyi]

I looks like the second stack trace I mentioned is due to getStack from dwds returning null (which should only happen when the application is not paused), but I cannot repro it on my local machines.

DWDS should return an error response instead of null since package:vm_service always expects a valid JSON response, except in the situation where the actual connection goes down. Does streamListen do something similar?

[annagrin]

DWDS should return an error response instead of null since package:vm_service always expects a valid JSON response, except in the situation where the actual connection goes down.

Will fix, thanks! This seems like a different issue, I created a new one:
dart-lang/webdev#1369

Does streamListen do something similar?

No, it returns a stream or an RPC error. I think this is a genuine DDS issue since it does not happen when I disable DDS.

[bkonyi]

So it looks like we've got some race conditions in the StreamManager in DDS. Since validateFlutterVersion calls streamListen and then almost immediately calls streamCancel, calling it twice in parallel can get us into a situation where client A is attempting to listen to the stream and client B cancels the stream, cleaning up state in DDS after we've checked that the stream is already listened to by DDS while handling client A's request.

We need to do some sort of locking around the global state in DDS to make sure things aren't cleaned up while there's multiple requests in flight.

[annagrin]

@bkonyi I am curious about locking mechanisms you use - I was able to fix a few race conditions in dwds by making operations atomic using map.putIfAbsent to store futures (for adding and removing breakpoints), or using AsyncMemoizer for initialization of various data structures. I would like to learn more about how those problems are usually handled in dart.

[annagrin]

Re-opening since the fix was reverted, the currently disabled test in flutter still fails if enabled:

https://github.com/flutter/flutter/blob/c49eba6c3f0c6d7075ceec3ce3a805e8e86fb48c/packages/flutter_tools/test/web.shard/vm_service_web_test.dart#L65

[bkonyi]

This has since relanded in this CL.

[annagrin]

Thanks, this fixes the failing test in flutter tools!