WIP on new stockfish api

lib/src/binding.dart

@@ -4,7 +4,7 @@ import 'package:firebase_messaging/firebase_messaging.dart';
 import 'package:flutter/foundation.dart';
 import 'package:flutter/widgets.dart';
 import 'package:lichess_mobile/firebase_options.dart';
-import 'package:lichess_mobile/src/model/engine/engine.dart';
+import 'package:multistockfish/multistockfish.dart';
 import 'package:shared_preferences/shared_preferences.dart';
 
 /// A singleton class that provides access to plugins and external APIs.
@@ -82,8 +82,8 @@ abstract class LichessBinding {
   /// Wraps [FirebaseMessaging.onBackgroundMessage].
   void firebaseMessagingOnBackgroundMessage(BackgroundMessageHandler handler);
 
-  /// The factory to create Stockfish
-  StockfishFactory get stockfishFactory;
+  /// The Stockfish singleton instance.
+  Stockfish get stockfish;
 }
 
 /// A concrete implementation of [LichessBinding] for the app.
@@ -169,5 +169,5 @@ class AppLichessBinding extends LichessBinding {
       FirebaseMessaging.onMessageOpenedApp;
 
   @override
-  StockfishFactory get stockfishFactory => const StockfishFactory();
+  Stockfish get stockfish => Stockfish.instance;
 }

lib/src/model/engine/engine.dart

@@ -28,9 +28,6 @@ abstract class Engine {
   /// Stop the engine current computation.
   void stop();
 
-  /// A future that completes once the underlying engine process is exited.
-  Future<void> get exited;
-
   /// Whether the engine is disposed.
   ///
   /// This will be `true` once [dispose] is called. Once the engine is disposed, it cannot be
@@ -39,10 +36,8 @@ abstract class Engine {
 
   /// Dispose the engine. It cannot be used after this method is called.
   ///
-  /// Returns the same future as [exited], that completes once the underlying engine process is exited.
-  ///
   /// It is safe to call this method multiple times.
-  Future<void> dispose();
+  void dispose();
 }
 
 const _nnueDownloadUrl = '$kLichessCDNHost/assets/lifat/nnue/';
@@ -75,28 +70,23 @@ class StockfishEngine implements Engine {
   final String? _smallNetPath;
   final String? _bigNetPath;
 
-  Stockfish? _stockfish;
+  Stockfish get _stockfish => LichessBinding.instance.stockfish;
+
   String _name = 'Stockfish';
   StreamSubscription<String>? _stdoutSubscription;
-
   bool _isDisposed = false;
+  bool _initInProgress = false;
 
   final _state = ValueNotifier(EngineState.initial);
 
   final _log = Logger('StockfishEngine');
 
-  /// A completer that completes once the underlying engine has exited.
-  final _exitCompleter = Completer<void>();
-
   @override
   ValueListenable<EngineState> get state => _state;
 
   @override
   String get name => _name;
 
-  @override
-  Future<void> get exited => _exitCompleter.future;
-
   @override
   bool get isDisposed => _isDisposed;
 
@@ -109,75 +99,55 @@ class StockfishEngine implements Engine {
     _log.info('engine start at ply ${work.position.ply} and path ${work.path}');
     _protocol.compute(work);
 
-    if (_stockfish == null) {
-      try {
-        final stockfish = LichessBinding.instance.stockfishFactory(
-          flavor,
-          smallNetPath: _smallNetPath,
-          bigNetPath: _bigNetPath,
-        );
-        _stockfish = stockfish;
-
-        _state.value = EngineState.loading;
-        _stdoutSubscription = stockfish.stdout.listen((line) {
-          _protocol.received(line);
-        });
-
-        stockfish.state.addListener(_stockfishStateListener);
-
-        // Ensure the engine is ready before sending commands
-        void onReadyOnce() {
-          if (stockfish.state.value == StockfishState.ready) {
-            _protocol.connected((String cmd) {
-              stockfish.stdin = cmd;
-            });
-            stockfish.state.removeListener(onReadyOnce);
-          }
-        }
-
-        stockfish.state.addListener(onReadyOnce);
-
-        // Check immediately in case the engine is already ready
-        // This prevents a race where the engine becomes ready between
-        // adding _stockfishStateListener and adding onReadyOnce
-        if (stockfish.state.value == StockfishState.ready) {
-          onReadyOnce();
-        }
-
-        _protocol.isComputing.addListener(() {
-          if (_protocol.isComputing.value) {
-            _state.value = EngineState.computing;
-          } else {
-            _state.value = EngineState.idle;
-          }
-        });
-        _protocol.engineName.then((name) {
-          _name = name;
-        });
-      } catch (e, s) {
-        _stockfish = null;
-        _log.severe('error loading stockfish', e, s);
-        _state.value = EngineState.error;
-      }
+    if (!_initInProgress && _stockfish.state.value != StockfishState.ready) {
+      _initInProgress = true;
+      _state.value = EngineState.loading;
+      _initStockfish();
     }
 
     return _protocol.evalStream.where((e) => e.$1 == work);
   }
 
+  Future<void> _initStockfish() async {
+    try {
+      _stockfish.state.addListener(_stockfishStateListener);
+
+      await _stockfish.quit();
+      if (isDisposed) return;
+
+      await _stockfish.start(
+        flavor: flavor,
+        smallNetPath: _smallNetPath,
+        bigNetPath: _bigNetPath,
+      );
+      if (isDisposed) return;
+
+      _stdoutSubscription = _stockfish.stdout.listen(_protocol.received);
+      _protocol.connected((cmd) => _stockfish.stdin = cmd);
+      _protocol.isComputing.addListener(_computingListener);
+      _protocol.engineName.then((name) => _name = name);
+    } catch (e, s) {
+      if (!isDisposed) {
+        _log.severe('error starting stockfish', e, s);
+        _state.value = EngineState.error;
+      }
+    } finally {
+      _initInProgress = false;
+    }
+  }
+
+  void _computingListener() {
+    _state.value = _protocol.isComputing.value ? EngineState.computing : EngineState.idle;
+  }
+
   void _stockfishStateListener() {
-    switch (_stockfish?.state.value) {
+    switch (_stockfish.state.value) {
       case StockfishState.ready:
         _state.value = EngineState.idle;
       case StockfishState.error:
         _state.value = EngineState.error;
-      case StockfishState.disposed:
-        _log.info('engine disposed');
-        _state.value = EngineState.disposed;
-        _exitCompleter.complete();
-        _stockfish?.state.removeListener(_stockfishStateListener);
-        _state.dispose();
       default:
-      // do nothing
+        break;
     }
   }
 
@@ -190,65 +160,15 @@ class StockfishEngine implements Engine {
   }
 
   @override
-  Future<void> dispose() {
-    if (isDisposed) {
-      return exited;
-    }
+  void dispose() {
+    if (isDisposed) return;
     _log.fine('disposing engine');
     _isDisposed = true;
     _stdoutSubscription?.cancel();
     _protocol.dispose();
-    if (_stockfish != null) {
-      switch (_stockfish!.state.value) {
-        case StockfishState.disposed:
-        case StockfishState.error:
-          if (_exitCompleter.isCompleted == false) {
-            _exitCompleter.complete();
-          }
-        case StockfishState.ready:
-          _stockfish!.dispose();
-        case StockfishState.initial:
-        case StockfishState.starting:
-          // wait to be ready or error, then dispose
-          void onReadyOrErrorOnce() {
-            final currentState = _stockfish!.state.value;
-            if (currentState == StockfishState.ready) {
-              _stockfish!.dispose();
-              _stockfish!.state.removeListener(onReadyOrErrorOnce);
-            } else if (currentState == StockfishState.error ||
-                currentState == StockfishState.disposed) {
-              if (_exitCompleter.isCompleted == false) {
-                _exitCompleter.complete();
-              }
-              _stockfish!.state.removeListener(onReadyOrErrorOnce);
-            }
-          }
-          _stockfish!.state.addListener(onReadyOrErrorOnce);
-          // Check immediately in case state already transitioned
-          onReadyOrErrorOnce();
-      }
-    } else {
-      if (_exitCompleter.isCompleted == false) {
-        _exitCompleter.complete();
-      }
-    }
-    return exited;
+    _stockfish.state.removeListener(_stockfishStateListener);
+    _stockfish.quit();
+    _state.value = EngineState.disposed;
+    _state.dispose();
   }
 }
-
-/// A factory to create a [Stockfish].
-///
-/// This is useful to be able to mock [Stockfish] in tests.
-class StockfishFactory {
-  const StockfishFactory();
-
-  Stockfish call(
-    StockfishFlavor flavor, {
-
-    /// Full path to the small net file for NNUE evaluation.
-    String? smallNetPath,
-
-    /// Full path to the big net file for NNUE evaluation.
-    String? bigNetPath,
-  }) => Stockfish(flavor: flavor, smallNetPath: smallNetPath, bigNetPath: bigNetPath);
-}

lib/src/model/engine/evaluation_mixin.dart

@@ -110,7 +110,7 @@ mixin EngineEvaluationMixin<T extends EvaluationMixinState> on AnyNotifier<Async
     if (state.requireValue.isEngineAvailable(evaluationPrefs)) {
       requestEval(forceRestart: true);
     } else {
-      await _evaluationService.disposeEngine();
+      _evaluationService.disposeEngine();
     }
   }
 

lib/src/model/engine/evaluation_service.dart

@@ -128,7 +128,7 @@ class EvaluationService {
   /// This method must be called before calling [start]. It is the caller's
   /// responsibility to close the engine.
   Future<void> _initEngine(EvaluationContext context, {EvaluationOptions? initOptions}) async {
-    await disposeEngine();
+    disposeEngine();
     _context = context;
     if (initOptions != null) options = initOptions;
     ChessEnginePref pref = options.enginePref;
@@ -181,13 +181,11 @@ class EvaluationService {
 
   /// Dispose the engine.
   ///
-  /// Returns a future that completes once the engine is disposed.
   /// It is safe to call this method multiple times.
-  Future<void> disposeEngine() {
-    return (_engine?.dispose() ?? Future.value()).then((_) {
-      _engine = null;
-      _state.value = _defaultState;
-    });
+  void disposeEngine() {
+    _engine?.dispose();
+    _engine = null;
+    _state.value = _defaultState;
   }
 
   /// Dispose the service.

pubspec.lock

@@ -1089,11 +1089,10 @@ packages:
   multistockfish:
     dependency: "direct main"
     description:
-      name: multistockfish
-      sha256: "825a16c840b42a415f52a1a026d6a73eaef0dbdaf725cf021bc3b87633a08178"
-      url: "https://pub.dev"
-    source: hosted
-    version: "0.3.0"
+      path: "../dart-multistockfish/pkgs/multistockfish"
+      relative: true
+    source: path
+    version: "0.4.0"
   multistockfish_chess:
     dependency: transitive
     description:

pubspec.yaml

@@ -56,7 +56,9 @@ dependencies:
   material_color_utilities: ^0.11.1
   material_symbols_icons: ^4.2811.0
   meta: ^1.8.0
-  multistockfish: ^0.3.0
+  # multistockfish: ^0.3.0
+  multistockfish:
+    path: ../dart-multistockfish/pkgs/multistockfish
   package_info_plus: ^9.0.0
   path: ^1.8.2
   path_provider: ^2.1.5

test/binding.dart

@@ -5,8 +5,8 @@ import 'package:firebase_messaging/firebase_messaging.dart';
 import 'package:flutter_test/flutter_test.dart';
 import 'package:intl/intl.dart';
 import 'package:lichess_mobile/src/binding.dart';
-import 'package:lichess_mobile/src/model/engine/engine.dart';
 import 'package:logging/logging.dart';
+import 'package:multistockfish/multistockfish.dart';
 import 'package:shared_preferences/shared_preferences.dart';
 
 import 'model/engine/fake_stockfish.dart';
@@ -115,13 +115,13 @@ class TestLichessBinding extends LichessBinding {
   Stream<RemoteMessage> get firebaseMessagingOnMessageOpenedApp =>
       firebaseMessaging.onMessageOpenedApp.stream;
 
-  StockfishFactory _stockfishFactory = const FakeStockfishFactory();
+  Stockfish _stockfish = FakeStockfish();
 
   @override
-  StockfishFactory get stockfishFactory => _stockfishFactory;
+  Stockfish get stockfish => _stockfish;
 
-  set stockfishFactory(StockfishFactory factory) {
-    _stockfishFactory = factory;
+  set stockfish(Stockfish instance) {
+    _stockfish = instance;
   }
 }
 

test/model/engine/engine_test.dart

@@ -16,8 +16,7 @@ void main() {
   TestLichessBinding.ensureInitialized();
 
   setUp(() {
-    final testBinding = TestLichessBinding.instance;
-    testBinding.stockfishFactory = const FakeStockfishFactory();
+    testBinding.stockfish = FakeStockfish();
   });
 
   group('Engine', () {
@@ -40,10 +39,10 @@ void main() {
       expect(eval.bestMove, const NormalMove(from: Square.e2, to: Square.e4));
     });
 
-    test('Dispose completes even when engine transitions to error state', () {
+    test('Dispose works when engine transitions to error state', () {
       fakeAsync((async) {
-        final errorStockfish = ErrorStockfish(StockfishFlavor.variant);
-        testBinding.stockfishFactory = FakeStockfishFactory(errorStockfish);
+        final errorStockfish = ErrorStockfish();
+        testBinding.stockfish = errorStockfish;
 
         final stockfishEngine = StockfishEngine(StockfishFlavor.variant);
 
@@ -60,15 +59,14 @@ void main() {
 
         stockfishEngine.start(work);
 
-        final disposeFuture = stockfishEngine.dispose();
-
         async.flushMicrotasks();
 
-        expectLater(disposeFuture, completes);
+        stockfishEngine.dispose();
 
         async.elapse(const Duration(seconds: 1));
 
-        expect(stockfishEngine.state.value, EngineState.error);
+        expect(stockfishEngine.state.value, EngineState.disposed);
+        expect(stockfishEngine.isDisposed, isTrue);
       });
     });
   });

test/model/engine/evaluation_service_test.dart

@@ -92,7 +92,7 @@ void main() {
       // Options should still match the first initialization
       expect(service.options, options);
 
-      await service.disposeEngine();
+      service.disposeEngine();
     });
 
     test('Sequential engine initializations are allowed', () async {
@@ -124,7 +124,7 @@ void main() {
       // Second call should be allowed since first completed
       expect(service.options, options2);
 
-      await service.disposeEngine();
+      service.disposeEngine();
     });
   });
 }

test/model/engine/fake_stockfish.dart

@@ -1,42 +1,20 @@
 import 'dart:async';
 
 import 'package:dartchess/dartchess.dart';
-import 'package:flutter/src/foundation/change_notifier.dart';
-import 'package:lichess_mobile/src/model/engine/engine.dart';
+import 'package:flutter/foundation.dart';
 import 'package:multistockfish/multistockfish.dart';
 
-class FakeStockfishFactory extends StockfishFactory {
-  const FakeStockfishFactory([this._instance]);
-
-  final Stockfish? _instance;
-
-  @override
-  Stockfish call(StockfishFlavor flavor, {String? bigNetPath, String? smallNetPath}) =>
-      _instance ?? FakeStockfish(flavor);
-}
-
-/// A fake implementation of [Stockfish].
+/// A fake implementation of [Stockfish] for testing.
 class FakeStockfish implements Stockfish {
-  FakeStockfish(this.flavor) {
-    scheduleMicrotask(() {
-      _state.value = StockfishState.ready;
-    });
-  }
+  FakeStockfish();
 
-  @override
-  String? get variant => null;
+  final _state = ValueNotifier<StockfishState>(StockfishState.initial);
+  final _stdoutController = StreamController<String>.broadcast();
 
-  @override
-  String? get bigNetPath => throw UnimplementedError();
-
-  @override
-  String? get smallNetPath => throw UnimplementedError();
-
-  @override
-  final StockfishFlavor flavor;
-
-  final _state = ValueNotifier<StockfishState>(StockfishState.starting);
-  final _stdoutController = StreamController<String>();
+  StockfishFlavor _flavor = StockfishFlavor.sf16;
+  String? _variant;
+  String? _smallNetPath;
+  String? _bigNetPath;
 
   Position? _position;
 
@@ -46,6 +24,39 @@ class FakeStockfish implements Stockfish {
     });
   }
 
+  @override
+  StockfishFlavor get flavor => _flavor;
+
+  @override
+  String? get variant => _variant;
+
+  @override
+  String? get bigNetPath => _bigNetPath;
+
+  @override
+  String? get smallNetPath => _smallNetPath;
+
+  @override
+  Future<void> start({
+    StockfishFlavor flavor = StockfishFlavor.sf16,
+    String? variant,
+    String? smallNetPath,
+    String? bigNetPath,
+  }) async {
+    _flavor = flavor;
+    _variant = variant;
+    _smallNetPath = smallNetPath;
+    _bigNetPath = bigNetPath;
+    _state.value = StockfishState.starting;
+    await Future.microtask(() {});
+    _state.value = StockfishState.ready;
+  }
+
+  @override
+  Future<void> quit() async {
+    _state.value = StockfishState.initial;
+  }
+
   @override
   set stdin(String line) {
     final parts = line.trim().split(RegExp(r'\s+'));
@@ -96,9 +107,6 @@ class FakeStockfish implements Stockfish {
     }
   }
 
-  @override
-  void dispose() {}
-
   @override
   ValueListenable<StockfishState> get state => _state;
 
@@ -109,35 +117,45 @@ class FakeStockfish implements Stockfish {
 /// A fake Stockfish that transitions to error state instead of ready.
 /// This simulates initialization failure scenarios.
 class ErrorStockfish implements Stockfish {
-  ErrorStockfish(this.flavor) {
-    scheduleMicrotask(() {
-      _state.value = StockfishState.error;
-    });
-  }
+  ErrorStockfish();
+
+  final _state = ValueNotifier<StockfishState>(StockfishState.initial);
+  final _stdoutController = StreamController<String>.broadcast();
+
+  @override
+  StockfishFlavor get flavor => StockfishFlavor.sf16;
 
   @override
   String? get variant => null;
 
   @override
-  String? get bigNetPath => throw UnimplementedError();
+  String? get bigNetPath => null;
 
   @override
-  String? get smallNetPath => throw UnimplementedError();
+  String? get smallNetPath => null;
 
   @override
-  final StockfishFlavor flavor;
+  Future<void> start({
+    StockfishFlavor flavor = StockfishFlavor.sf16,
+    String? variant,
+    String? smallNetPath,
+    String? bigNetPath,
+  }) async {
+    _state.value = StockfishState.starting;
+    await Future.microtask(() {});
+    _state.value = StockfishState.error;
+  }
 
-  final _state = ValueNotifier<StockfishState>(StockfishState.starting);
-  final _stdoutController = StreamController<String>();
+  @override
+  Future<void> quit() async {
+    _state.value = StockfishState.initial;
+  }
 
   @override
   set stdin(String line) {
     // Do nothing - engine is in error state
   }
 
-  @override
-  void dispose() {}
-
   @override
   ValueListenable<StockfishState> get state => _state;