dart-archive / isolate

Makes working with Dart isolates easier.
https://pub.dev/packages/isolate
BSD 3-Clause "New" or "Revised" License
90 stars 34 forks source link

Tweak null safety #54

Closed lrhn closed 3 years ago

lrhn commented 3 years ago

Change the original migration of singleResponsePort and singleResponseFuture, instead of splitting into two methods, one "WithTimeout" and one "WithoutTimeout", drop the "WithoutTimeout" and use the original for that while deprecating the timeout parameters.

Undo the introduction of a separate PriorityQueue implementation, and modify the existing internal implementation in LoadBalanceer to be even more tightly tailored to the use. This avoided needing a nullable type and lots of !s since the actual queue never really changes size (you can't add runners to the pool after creation).

Minor tweaks to code where it seemed possible to do better (or just use more modern features than when the code was originally written).

Switches to using package:lints.

Fox32 commented 3 years ago

I gave the current state a try and pulled the version into my project as a Git dependency. Some of my local tests are running into timeouts, till now I could pin it down to timeout in LoadBalancer.close(). But the timeout only occures if you run at least one task inside the load balancer.

Here is a minimal reproducible example.

import 'package:isolate/isolate.dart';
import 'package:test/test.dart';

void main() {
  test('test', () async {
    final lb = await LoadBalancer.create(1, () => IsolateRunner.spawn());
    await lb.run(runTask, null);
    print('Closing');
    await lb.close();
    // Never executed, test runs into timeout after 30 seconds
    print('Closed');
  });
}

void runTask(_) {
  print('Isolate');
}

This works in 2.0.3 but not in master. On this branch (tweak-null-safety) it crashes instead:

dart:core                                  _AssertionError._throwNew
package:isolate/load_balancer.dart 270:12  LoadBalancer._decreaseLoad
package:isolate/load_balancer.dart 307:16  _LoadBalancerEntry.run.<fn>

'package:isolate/load_balancer.dart': Failed assertion: line 270 pos 12: 'load >= 0': is not true.

I couldn't find any tests for the load balancer in this repo, so I guess closing one isn't covered yet.

lrhn commented 3 years ago

@Fox32 Good catch. It's a silly bug where there _decrementLoad double-negates the load, both at the call point and inside the function. Exactly what the assert is there to catch. I should run tests with assertions enabled!

It's also true that the load-balancer is very much untested. I'll do something about that.

Fox32 commented 3 years ago

Another small thing I notice, but I'm not sure about. The return value of LoadBalancer.runMultiple has changed from Future<T> to FutureOr<T>. As it internally calls LoadBalancer.run which returns Future<T> I don't see why it should ever return something else than a Future<T>.

lrhn commented 3 years ago

I've tried writing something with LoadBalancer now, and the List<FutureOr<T>> result type is very annoying. It should be changed to List<Future<T>>, so you can pass it directly to Future.wait.

Fox32 commented 3 years ago

Just gave it a try in our code again: The state of this PR works fine after migration 👍

lrhn commented 3 years ago

Excellent!