-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
[breaking change] Remove dart:cli waitFor experiment #52121
Comments
I think it's a good idea that the Dart team provides the the low-level mutex and message-passing package, implemented using I'd rather not have user-written Dart code depend on pthreads being available in the We should make sure the package is designed abstractly enough, so we don't bind ourselves to a particular mutex implementation. (And we should document that in the package, so future maintainers won't start providing more implementation specific primitives.) LGTM. |
I was somehow thinking that we only do stable release every 6 months, but it seems that target cadence is quarterly. I am happy to commit to removing it in 3.4 instead of 3.2 to give full year. Updated the issue text to reflect this timeline. |
Implicitly through sass but not directly. The sass impact is constrained to a single file. |
This seems like a good path forward. Thanks, @mraleph. To simplify downstream compatibility, it would be helpful to have at least one release where both I'm also OK with not flagging it at all, but simply removing it after a couple of releases where we've had the migration options available. |
This doesn't meet the bar of providing a viable alternative for existing use-cases. Even if this synchronous isolate package addresses Sass's needs—which is still totally hypothetical at this point—it definitely doesn't meet dcli's. You say "the usage of asynchronous A viable alternative for this feature that addresses all its uses looks like fibers, for which there's a considerable amount of prior art of integrating it into evented systems like Dart. But if you're not ready to go there, please leave thing as they are as you said in #39390 (comment). |
@nex3 Mitigation section provides a number of migration paths which together address all existing uses of
Unless I am misunderstanding something this is also addressed by
If we only had native to worry about we would probably already done fibers. Unfortunately JS throws a wrench in this: adding fibers means sacrificing portability. My current thinking here is that the best we could do without sacrificing portability is to have fibers underneath async-await implementation (so that async-to-async calls become as cheap as normal calls), but that would not allow people to suspend execution across synchronous frames. This is anyway tangential to the desire to remove |
I understand that you're eager to get rid of "Mitigation" isn't the same thing as a "viable alternative", though. Have you discussed with @bsutton or other maintainers of tools using
The proposed solution addresses 1 acceptably, but it doesn't address 2 without a huge amount of additional wrapper work on the part of users and it doesn't address 3 at all. All of these are exacerbated by the fact that "single-threaded" async code is much slower than the equivalent synchronous code even when there's no IO going on.
My expectation would be that Fibers would be a VM-only feature, like |
So I guess I should comment.
I don't see the proposed mitigations as being viable.
As someone that builds dcli in my spare time this looks to be a huge effort.
I have consider removing waitfor from dcli but this is a huge breaking
change and we still have the problem that the linter doesn't cover all of
the 'you forgot to await x' scenarios which makes writing cli apps using an
async library dangerous.
If we had full linter coverage I would consider going async as more viable
than the proposed path.
Now fibers is something I would be interested in.
…On Sat, 22 Apr 2023, 7:18 am Natalie Weizenbaum, ***@***.***> wrote:
I understand that you're eager to get rid of waitFor. It's definitely not
the right abstraction for what it's trying to do—I agree with that
completely. But please don't let that eagerness translate into putting
users into an untenable situation. We all want to find *the right*
alternative here.
"Mitigation" isn't the same thing as a "viable alternative", though. Have
you discussed with @bsutton <https://github.com/bsutton> or other
maintainers of tools using waitFor to see if they consider this viable? I
don't want to put words in their mouth, but I'd be very surprised if they
find the prospect of re-implementing and maintaining large dart:io APIs
as isolate proxies to be a good solution.
waitFor() provides three important capabilities that are otherwise
missing from Dart today:
1.
Allowing async code to be passed as callbacks to synchronous APIs.
This is essentially the Sass embedded compiler's use-case.
2.
Providing synchronous access to dart:io APIs that are async-only, such
as Process.start, HTTP, sockets, etc. This is dcli's primary use case.
3.
Making it possible to share logic across synchronous and asynchronous
APIs. Even when both APIs exist, there's no other way to write code that
can operate either synchronously or asynchronously other than copying it
and everything it transitively touches. This comes up occasionally for
Google-internal scripting.
The proposed solution addresses 1 acceptably, but it doesn't address 2
without a huge amount of additional wrapper work on the part of users and
it doesn't address 3 at all. All of these are exacerbated by the fact that
"single-threaded" async code is *much* slower than the equivalent
synchronous code even when there's no IO going on.
If we only had native to worry about we would probably already done
fibers. Unfortunately JS throws a wrench in this: adding fibers means
sacrificing portability.
My expectation would be that Fibers would be a VM-only feature, like
waitFor() is today. Everything we're talking about—isolates, dart:io,
dart:ffi—is already VM-only anyway, so it seems like it would make sense
to give the VM special functionality for interacting with the event loop in
more powerful ways.
—
Reply to this email directly, view it on GitHub
<#52121 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OCUTNRWCNEVNKX5CWDXCL2TBANCNFSM6AAAAAAXGRJJN4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Do you expect this to be provided by the Dart analyzer / linter or are open to accept external solutions as well? |
I view this a fundamental requirement and believe it should be in the dart
linter and in the recommended set of lints in the lints package.
Even for flutter apps tracking down a missing await is expensive.
I suspect that are many flutter apps with subtle behaviour problems the dev
team can never nail down. I've had a few of these over the years and you
waste hours on them.
Missing awaits are also a code smell. You can't tell what the original devs
intention was, should it be unawaited or did they just forget.
…On Sat, 22 Apr 2023, 5:04 pm Dmitry Zhifarsky, ***@***.***> wrote:
we still have the problem that the linter doesn't cover all of
the 'you forgot to await x' scenarios which makes writing cli apps using an
async library dangerous.
Do you expect this to be provided by the Dart analyzer / linter or are
open to accept external solutions as well?
—
Reply to this email directly, view it on GitHub
<#52121 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OCHUWUJ7HWUMH2MEDDXCN7GLANCNFSM6AAAAAAXGRJJN4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sorry, but this reads as if your intentions are to get this rule in the linter and the core set, not to find any good option that unblocks the migration. If this is such a problem, why you're focused only on one specific solution? |
You asked if I thought it should be included in dart, so that was the
context of my answer.
My answer in terms of unblocking this problem is however still the same.
Dcli is a library used by other parties.
The need to be able to use it safely.
A missing await in dcli can result in critical data being deleted.
I would find it problematic for users of dcli to have to go off and find
some third party solution.
Most people don't read the documentation until is too late.
if there is a single solution that fixes multiple problems then I always
consider that the better solution.
There is already work on the linter to improve this area, it just needs to
be completed and will benefit everyone.
This path also seems like the least amount of work for everyone and also
feels like 'the right' solution.
…On Sat, 22 Apr 2023, 5:58 pm Dmitry Zhifarsky, ***@***.***> wrote:
I view this a fundamental requirement and believe it should be in the dart
linter and in the recommended set of lints in the lints package.
Sorry, but this reads as if your intentions are to get this rule in the
linter and the core set, not to find any good option that unblocks the
migration. If this is such a problem, why you're focused only on one
specific solution?
—
Reply to this email directly, view it on GitHub
<#52121 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OBZA2DLZGE2YIHTD4DXCOFSHANCNFSM6AAAAAAXGRJJN4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Fair point. Now I better understand the reasons behind it, thank you. |
@pq - do we have a pointer to the linter work in this area (unawaited async)? |
@bsutton I'm curious to know -- is there a good list of sync APIs you'd need that might make it easier to migrate from That is, can you expand on @nex3's comment here?
In my own package, I can drop |
@bsutton, regarding lint support, would a combination of unawaited_futures and discarded_futures be sufficient? (Assuming false positives are addressed in the latter?) |
I have spent time trying to understand implications for both https://github.com/sass/dart-sass-embedded and https://github.com/onepub-dev/dcli by eliminating
|
Issue for addressing this inefficiency: sass/sass#3579
That's fair, but I suggest to not rush setting timeline for removing Deprecation can be declared early to suggest new user to not use a to-be removed feature, but obsoletion timeline should only be declared when the alternative solution is production ready. Declare an obsoletion when all the alternative solutions are still just POC is way too aggressive. If Dart team is committed to build the alternative that is needed for migration, I think it won't be too late to talk about obsoletion timeline after you have most developers successfully migrate off the deprecated feature. |
POC from yesterday worked on any POSIX system (originally developed on Mac OS X, but will also work on Linux, iOS and Android). I spend ~1h today abstracting the code a bit and porting it to Windows - which was fairly trivial as well just as one would expect. All
The My experiments demonstrate without any doubt that Dart already today provides all necessary tools for users to migrate off |
We (the Sass and dcli teams) are trying to be patient and work with the Dart team to ensure that we can find a solution we're all happy with to move forward here. I think we've been quite clear about what the critical parts of the status quo are for us, and we've provided a number of possible solutions to move forward that would satisfy our needs. I don't feel like we're getting the same patience or care from you, @mraleph. Unilaterally moving the goalposts from "when we have a viable alternative" to a drop-dead date is an aggressive, anti-collaborative tactic. It makes me, as a user, worry that my needs or even commitments made by the Dart team are not relevant at all to the decision-making that actually happens. It also makes me feel like the effort we've put into working with you in good faith to try and find a mutually satisfactory solution was wasted. This isn't just an academic exercise. There are real people who rely on this software— |
@nex3 - @mraleph is digging into the code to propose viable alternatives above. We'd all like reasonable off ramps for sass and dcli. I don't think we need a general purpose alternative such as fibers to move forward. As you've noted, V8 chose to not go that way: https://sass-lang.com/blog/node-fibers-discontinued#the-death-of-fibers |
@nex3 I am sorry you feel this way. I would like to understand which part of this you don't consider viable. When reading that code you have to assume that I also encourage you to see timeline from our perspective:
|
@bsutton One (simplest) solution for Another solution (more involved one) could be to use FFI and implement named lock on top of platform specific named mutex APIs: |
Revert "[vm] Enable waitFor for 3.2 release" This reverts commit f8086ed. R=kustermann@google.com TEST=covered by existing tests Bug: #52121 Change-Id: Ic5c7ad9af8ff142564ce237f868dd39354dfd615 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/338761 Reviewed-by: Martin Kustermann <kustermann@google.com> Commit-Queue: Slava Egorov <vegorov@google.com>
Hi, I'm using // testing cloud_functions_core
final pccGet = waitFor<Process>(Process.start(
'dart',
['test', '--chain-stack-traces'],
mode: ProcessStartMode.inheritStdio,
workingDirectory: '../cloud_functions_core',
environment: {'DOCKER_BUILDKIT': '1'},
));
if (waitFor<int>(pccGet.exitCode) != 0) {
stderr.writeln('ERROR: Testing failed.');
exit(1);
}
Thanks |
@jimmyff |
Ah, now I feel silly -that's exactly when I needed. Sorry for noise! |
The removal is complete. |
@mraleph I'm trying to attempt exactly this! Repository here: I'd absolutely love some feedback as this is a first stab at an implementation and using some pretty new stuff that I haven't used before 😅 I come from the GRPC + Protobuf / Traditional Backend World - haven't done too much on Windows FFI but giving it a really solid effort & hope to share this out there and get it into DCLI at some point. For unix it tries to use Flock and fctnl from the stdlibc package that Canonical just released on pub.dev. It also tries to wrap things in an extra layer of protection with Mutex from native_synchronization. |
I would push you back in the direction of the file create idea.
Flock won't work as there is no easy way to keep a lock count across
isolates.
The create method needs no other help, if you created the named lock file
then you own the lock.
…On Wed, 27 Mar 2024, 3:17 pm Tsavo Knott, ***@***.***> wrote:
@bsutton <https://github.com/bsutton> One (simplest) solution for
NamedLock problem would be to build your own waitFor out of Isolate.run
and package:native_synchronization: you create an isolate which is
responsible for managing ServerSocket instance and use blocking
synchronization primitives in the main isolate to work with it.
Another solution (more involved one) could be to use FFI and implement
named lock on top of platform specific named mutex APIs: CreateMutexW on
Windows and sem_open (or PTHREAD_PROCESS_SHARED mutexes residing in
shared memory) on POSIX OSes.
Attempting exactly this!
Repository here:
https://github.com/open-runtime/native_named_locks/blob/aot_monorepo_compat_dart_ffi/lib%2Fwindows_named_lock.dart
I'd absolutely love some feedback as this is a first stab at an
implementation and using some pretty new stuff that I haven't used before
😅 I come from the GRPC + Protobuf / Traditional Backend World - haven't
done too much on Windows FFI but giving it a really good try there!
For unix it tries to use Flock and fctnl from the stdlibc package that
Canonical just released on pub.dev.
It also tries to wrap things in an extra layer of protection with Mutex
from native_synchronization.
—
Reply to this email directly, view it on GitHub
<#52121 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OFXHVC2G2FWDCD753LY2JB4FAVCNFSM6AAAAAAXGRJJN6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRRHA4TGNRZHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Gentlefolk, I need some help as I've come to a grinding halt on converting dcli away from waitfor. The problem is with implementing synchronous process execution. @mraleph helped by providing the mailbox class which allows for sync data transfer between isolates. The problem is with spawning an isolate to run the process. Spawning an isolate is an async process but I can't use an await when spawning as that makes the call async. I spawn the process - without awaiting it - and then immediately called the synchronous mailbox.take method. The problem is that calling a sync method immediately after the spawn seems to stop the spawn from completing. pseudo code:
I'm hoping I've misses something because at the moment I don't see any way forward. The good news is that the rest of the conversion is essentially complete except for some cleanup. |
Hey @bsutton! Quick question here - is this what you were thinking about using named locks for? I have been meaning to post an update but I have it implemented and passing on MacOS, Linux and Windows with unit tests that verify cross-process synchronization. Apologies if this is unrelated.. can sync with you on the named locks back over on DCLI repo! Regarding the above - do you need to pass the mailbox as a sendable? I had to do something like that when I used it for the PR a little bit ago. Link here |
@tsavo-at-pieces no this isn't related to named locks. The issue isn't with passing the mailbox, I have that working. But yes we should talk about named locks - I've been a little distracted over the last few days. |
Okay solid - interesting situation here - are you able to share a git link to the problematic code? I'm wrapping up named locks tonight and can take a look at this as well (no promises but definitely happy to take a crack at it locally)! |
I'm assuming the you're calling something like mailbox.put within the isolate before calling mailbox.take? |
The code in question is here: After spawning isolate the call to _connectSendPort makes the call to take. I'm locally playing with some alternate patterns, but if you have any ideas I would be pleased to here them. |
I'm assuming the you're calling something like mailbox.put within the
isolate before calling mailbox.take?
This is the crux of the problem.
Because we can't 'await' the spawn we can't know if the mailbox has been
primed via a call to put.
Any sync call (from the primary isolate) to determine if the mailbox is
primed, stops the spawn completing.
Catch 22.
…On Wed, Apr 3, 2024 at 8:20 AM Tsavo Knott ***@***.***> wrote:
I'm assuming the you're calling something like mailbox.put within the
isolate before calling mailbox.take?
—
Reply to this email directly, view it on GitHub
<#52121 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OHFPT2QHCKIAZTX7ADY3MOIHAVCNFSM6AAAAAAXGRJJN6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZTGEYTQNZZGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@bsutton okay solid - I believe named locks can actually help here with something like the following usage.
|
6 is problematic. As we can't know if the spawned isolate ever took the
lock. So we would have to sleep (which slows launch down) and we still
have no guarantees.
But the real problem is that the sleep is sync which will stop the isolate
spawning.
I could be wrong but I don't think the isolate gets spawned until some
Async code executes. If this is correct the there is no way around the
problem.
I think we need some input from the dart Devs.
…On Wed, 3 Apr 2024, 9:29 am Tsavo Knott, ***@***.***> wrote:
@bsutton <https://github.com/bsutton> okay solid - I believe named locks
can actually help here with something like the following usage.
1. Create a NamedLock instance on the Main Thread and immediately lock
it
2. Pass in the name of the NamedLock to the Isolate through the _startIsolate(settings,
channel); call
3. Create a NamedLock instance within the Isolate Thread from the
passed name
4. The Isolate then tries to acquire the NamedLock with a blocking
NamedLock.acquire() call
5. Back on the main thread, right after the _startIsolate(settings,
channel); call, we call proceed to call NamedLock.unlock() from the
main thread allowing the isolate to successfully complete its
NamedLock.acquire() call, lock the NamedLock within the Isolate and
allow it to proceed and call mailbox.put()
6. Again on the main thread, we call NamedLock.acquire() ahead of our
mailbox.take() can introduce a small sleep if needed but this main
thread NamedLock.acquire() will block mailbox.take() until the isolate
calls NamedLock.unlock()
7. Lastly on the Isolate side right after it calls mailbox.put() it
proceeds to call NamedLock.unlock() or NamedLock.dispose() unblocking
the main threads blocking NamedLock.acquire() preceding the call to
mailbox.take()
—
Reply to this email directly, view it on GitHub
<#52121 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OC7UVVQAUEHL4SYO4LY3MWM3AVCNFSM6AAAAAAXGRJJN6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZTGIYDQOBSGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Absolutely - looking forward to input from Dart Devs as well! Last thing to note is that the NamedLocks implementation I've been working on has a native shared memory map/counter to count the number of threads/processes that have a NamedLocks instance with an unresolved |
I've raised this issue to create spawnSync as I think it is the only workable solution: |
Awesome - good looks 🫡 Looking forward to hearing more from the Dart team! We'll get there - it's always a bit frustrating in the moment but gonna be solid 🤝 appreciate you @bsutton! |
Deleted my comment after further research since I can see you want to interact with the spawned process in a sync matter with stdin and stdout. |
Change Intent
waitFor
contradicts Dart's event-loop model in a way that no other old or new feature does: it allows async and synchronous code to interleave. The feature has been marked experimental since the beginning and effectively has only two users: sass and dcli package. It is not available in Flutter or on the Web becausedart:cli
library is not exposed there.See discussion on #39390 for context.
Justification
We would like to reduce VM's technical debt by removing this experimental functionality, which we believe was added hastily and without due thought to the consequences.
We also have some indications (based on some internal code which has been migrated from
waitFor
) that availability ofwaitFor
encourages ineffecient and convoluted coding patterns.If you compare
waitFor
withFinalizer
(which was added in 2.17) you will observe that we choose to makeFinalizer
less powerful by specifying that finalizers are only invoked at the turn of the event loop for the sake of maintaining semantic purity around interleaving of synchronous code.Impact
Current users (which basically amounts to dcli and sass) will have to migrate off
waitFor
which will require them to rewrite their code.Mitigation
Based on our analysis of dcli and sass multiple migration strategies are available:
dart:io
methods can be replaced with synchronous counterparts, eliminating the need forwaitFor
;dart:ffi
primitives;Note that it is an explicit non-goal to provide a completely equivalent replacement for
waitFor
, because we believe the feature itself is incompatible with how Dart's async is designed and should be removed.Synchronous communication over dart:ffi
This code demonstrates how
dart:ffi
can be used to establish an entirely synchronous communication channel between the main (dispatcher) isolate and a worker isolate. This type of communication channel should cover sass needs.To make migration simpler we will provide a portable synchronization package which implements mutexes and conditional variables, though we leave implementation of cross-isolate communication channels to the developers.
Isolate.resolvePackageUri
Some minor users of
waitFor
use it to synchronously unwrapFuture
returned fromIsolate.resolvePackageUri
API. In reality the underlying implementation ofIsolate.resolvePackageUri
is entirely synchronous.While there is migration path for this code using
dart:ffi
and isolates we can simplify things by directly exposingIsolate.resolvePackageUriSync(...)
and deprecating async version of this API.Timeline
The removal will follow this timeline:
waitFor
feature was marked deprecatedwaitFor
is now disabled by default, but can be temporarily enabled by passing the flag--enable_deprecated_wait_for
to the VM /dart
commandwaitFor
and there will be no way of enabling itThe text was updated successfully, but these errors were encountered: