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.

Sign up for GitHub

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

Jump to bottom

Fix race condition between spawning and killing isolates during shutdown #2007

Merged
merged 5 commits into from
Jun 9, 2023
Merged

Fix race condition between spawning and killing isolates during shutdown #2007

merged 5 commits into from
Jun 9, 2023

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Jun 9, 2023

There is a race condition that can happen during shutdown:

  1. Host sends request
  2. Compiler spawns new isolate - the problem here is that this call is async
  3. Host closes compiler's stdin
  4. Compiler kills _allIsolates
  5. New isolate is spawned and added to _allIsolates
  6. ???

This PR makes sure the all isolates are killed correctly, however, it now throws an unhandled exception due to calling sink.add after close somewhere. It's not a big problem as it is just a noise for ungraceful shutdown.

Unhandled exception:
Bad state: Cannot add event after closing
#0      _StreamController.add (dart:async/stream_controller.dart:597:24)
#1      _StreamTransformerWrapperSink.add (package:async/src/stream_sink_transformer/stream_transformer_wrapper.dart:49:17)
#2      DelegatingStreamSink.add (package:async/src/delegate/stream_sink.dart:35:11)
#3      _RootZone.runUnaryGuarded (dart:async/zone.dart:1594:10)
#4      _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart:339:11)
#5      _BufferingStreamSubscription._add (dart:async/stream_impl.dart:271:7)
#6      _SinkTransformerStreamSubscription._add (dart:async/stream_transformers.dart:63:11)
#7      _EventSinkWrapper.add (dart:async/stream_transformers.dart:13:11)
#8      ExplicitCloseTransformer.bind.<anonymous closure> (package:sass/src/embedded/util/explicit_close_transformer.dart:28:14)
#9      _HandlerEventSink.add (dart:async/stream_transformers.dart:209:17)
#10     _SinkTransformerStreamSubscription._handleData (dart:async/stream_transformers.dart:111:24)
#11     _RootZone.runUnaryGuarded (dart:async/zone.dart:1594:10)
#12     _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart:339:11)
#13     _BufferingStreamSubscription._add (dart:async/stream_impl.dart:271:7)
#14     _SyncStreamControllerDispatch._sendData (dart:async/stream_controller.dart:776:19)
#15     _StreamController._add (dart:async/stream_controller.dart:650:7)
#16     _RootZone.runUnaryGuarded (dart:async/zone.dart:1594:10)
#17     _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart:339:11)
#18     _BufferingStreamSubscription._add (dart:async/stream_impl.dart:271:7)
#19     _SyncStreamControllerDispatch._sendData (dart:async/stream_controller.dart:776:19)
#20     _StreamController._add (dart:async/stream_controller.dart:650:7)
#21     _StreamController.add (dart:async/stream_controller.dart:598:5)
#22     _RootZone.runUnaryGuarded (dart:async/zone.dart:1594:10)
#23     _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart:339:11)
#24     _BufferingStreamSubscription._add (dart:async/stream_impl.dart:271:7)
#25     _SyncStreamControllerDispatch._sendData (dart:async/stream_controller.dart:776:19)
#26     _StreamController._add (dart:async/stream_controller.dart:650:7)
#27     _StreamController.add (dart:async/stream_controller.dart:598:5)
#28     _StreamSinkWrapper.add (dart:async/stream_controller.dart:868:13)
#29     _RootZone.runUnaryGuarded (dart:async/zone.dart:1594:10)
#30     CastStreamSubscription._onData (dart:_internal/async_cast.dart:85:11)
#31     _RootZone.runUnaryGuarded (dart:async/zone.dart:1594:10)
#32     _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart:339:11)
#33     _BufferingStreamSubscription._add (dart:async/stream_impl.dart:271:7)
#34     _SyncStreamControllerDispatch._sendData (dart:async/stream_controller.dart:776:19)
#35     _StreamController._add (dart:async/stream_controller.dart:650:7)
#36     _StreamController.add (dart:async/stream_controller.dart:598:5)
#37     _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:189:12)

@ntkme ntkme mentioned this pull request Jun 9, 2023
Closed
@nex3 nex3 self-requested a review June 9, 2023 21:30
@nex3
Copy link
Contributor

nex3 commented Jun 9, 2023

In trying to add a test for this I ended up uncovering a race condition in stream_channel itself, so I'm currently working on a fix for that.

nex3 added 4 commits June 9, 2023 15:21
@nex3
Update pubspec and changelog
88a5883
@nex3
Add a test
ed51d84 
This is marked skipped for now, because it also exposes a race
condition in `stream_channel` that's fixed by
dart-lang/stream_channel#92.
@nex3
Merge remote-tracking branch 'origin/main' into fix-shutdown-race-con…
99bc8ca 
…dition
@nex3
Merge branch 'fix-shutdown-race-condition' of github.com:ntkme/dart-s…
fa88455 
…ass into fix-shutdown-race-condition
@nex3 nex3 merged commit aa59a5f into sass:main Jun 9, 2023
@ntkme ntkme deleted the fix-shutdown-race-condition branch June 9, 2023 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nex3 nex3 nex3 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sass --embedded doesn't reliably exit when closing stdin
2 participants
@ntkme @nex3