mraleph commented 1 year ago

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 because dart:cli library is not exposed there.

See discussion on https://github.com/dart-lang/sdk/issues/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 of waitFor encourages ineffecient and convoluted coding patterns.

If you compare waitFor with Finalizer (which was added in 2.17) you will observe that we choose to make Finalizer 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:

The usage of asynchronous dart:io methods can be replaced with synchronous counterparts, eliminating the need for waitFor;
Asynchronous code can be moved to a helper isolate which communicates with the main isolate through synchronous communication channel built on top of low-level dart:ffi primitives;
Synchronous code be rewritten to become asynchronous, following Dart's canonical style for writing such code.

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 unwrap Future returned from Isolate.resolvePackageUri API. In reality the underlying implementation of Isolate.resolvePackageUri is entirely synchronous.

While there is migration path for this code using dart:ffi and isolates we can simplify things by directly exposing Isolate.resolvePackageUriSync(...) and deprecating async version of this API.

Timeline

The removal will follow this timeline:

Dart 2.15: The waitForfeature was marked deprecated
Dart 3.0: We announce the upcoming removal, offering ~1 year migration timeline
Dart 3.3: waitFor is now disabled by default, but can be temporarily enabled by passing the flag --enable_deprecated_wait_for to the VM / dart command
Dart 3.4: We'll fully remove waitFor and there will be no way of enabling it

mraleph commented 1 year ago

/cc @mit-mit @lrhn @mkustermann @vsmenon

itsjustkevin commented 1 year ago

@grouma does this affect your teams? @Hixie this mentions that it does not touch flutter.

@mraleph if we are targeting 3.2 this would most likely give users around 6 months to migrate.

lrhn commented 1 year ago

I think it's a good idea that the Dart team provides the the low-level mutex and message-passing package, implemented using dart:ffi. Probably as a tools package. That makes it possible for us to change the implementation in the future, if we, for example, stop using pthreads in the stand-alone VM.

I'd rather not have user-written Dart code depend on pthreads being available in the dart process. In fact, we should probably say somewhere that we do not guarantee such things.

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.

mraleph commented 1 year ago

@mraleph if we are targeting 3.2 this would most likely give users around 6 months to migrate.

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.

grouma commented 1 year ago

@grouma does this affect your teams?

Implicitly through sass but not directly. The sass impact is constrained to a single file.

timsneath commented 1 year ago

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 waitFor and the new sync APIs (such as resolvePackageUriSync) are available. That is, if we introduce resolvePackageUriSync in 3.1, we shouldn't flag waitFor in that same release.

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.

nex3 commented 1 year ago

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 dart:io methods can be replaced with synchronous counterparts", but there's no synchronous equivalent of many dart:io features, particularly running a process with interactive access to standard IO which dcli relies heavily on.

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 https://github.com/dart-lang/sdk/issues/39390#issuecomment-953779761.

mraleph commented 1 year ago

@nex3 Mitigation section provides a number of migration paths which together address all existing uses of waitFor. In my opinion primary migration path for dcli is to convert their code to use await, though other alternatives are also available.

there's no synchronous equivalent of many dart:io features, particularly running a process with interactive access to standard IO which dcli relies heavily on.

Unless I am misunderstanding something this is also addressed by dart:ffi and dart:isolate in the very same way you do it in SASS: simplest approach here is to spawn a worker isolate which uses asynchronous dart:io methods to run a process and handle IO and have this worker isolate communicate back to the main isolate using a synchronous channel built with dart:ffi.

A viable alternative for this feature that addresses all its uses looks like fibers

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 waitFor and has different timelines: fibers (even as an implementation detail) might or might not come, but we already have all the tools for current users to migrate of waitFor.

nex3 commented 1 year ago

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 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:

Allowing async code to be passed as callbacks to synchronous APIs. This is essentially the Sass embedded compiler's use-case.
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.
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.

bsutton commented 1 year ago

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 https://github.com/dart-lang/sdk/issues/52121#issuecomment-1518344527, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OCUTNRWCNEVNKX5CWDXCL2TBANCNFSM6AAAAAAXGRJJN4 . You are receiving this because you were mentioned.Message ID: @.***>

incendial commented 1 year ago

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?

bsutton commented 1 year ago

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 https://github.com/dart-lang/sdk/issues/52121#issuecomment-1518539543, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OCHUWUJ7HWUMH2MEDDXCN7GLANCNFSM6AAAAAAXGRJJN4 . You are receiving this because you were mentioned.Message ID: @.***>

incendial commented 1 year ago

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?

bsutton commented 1 year ago

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 https://github.com/dart-lang/sdk/issues/52121#issuecomment-1518552888, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OBZA2DLZGE2YIHTD4DXCOFSHANCNFSM6AAAAAAXGRJJN4 . You are receiving this because you were mentioned.Message ID: @.***>

incendial commented 1 year ago

Fair point. Now I better understand the reasons behind it, thank you.

vsmenon commented 1 year ago

@pq - do we have a pointer to the linter work in this area (unawaited async)?

timsneath commented 1 year ago

@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 waitFor?

That is, can you expand on @nex3's comment here?

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.

In my own package, I can drop waitFor altogether as soon as resolvePackageUriSync is exposed...

pq commented 1 year ago

@bsutton, regarding lint support, would a combination of unawaited_futures and discarded_futures be sufficient? (Assuming false positives are addressed in the latter?)

mraleph commented 1 year ago

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 waitFor myself.

`dart-sass-embedded`

Here is the branch that completely eliminates waitFor from dart-sass-embedded by switching it to an model where a new isolate is spawned for each incoming compilation request.

The main isolate works with stdin/stdout communication channel using asynchronous APIs and then dispatches compilations to worker isolates. When a worker isolate needs to call back to to host they send request back to the main isolate (using a normal SendPort) and suspends execution using native synchronisation primitives. Main isolate will then route response back to the worker isolate once it is received on the stdin.

I think this effectively repeats work that @nex3 and @ntkme have already done previously.

Unsurprisingly it works without any issues (all tests pass).

There is a bit too much serialization / deserialisation going on - which is probably wasteful (especially for large messages like compilation request / responses). This can be addressed by either changing protocol slightly or by changing how we deserialise these messages (protocol buffers allow you to peak inside them based on tag numbers).

My conclusion is that there are really no blockers for Sass here.

`dcli`

I have started going through dcli as well eliminating waitFor usage (here is snapshot of my changes). I have managed to mostly eliminate waitFor in the code with the exception of few large pieces and tests. The following patterns left unmigrated:

Synchronous flushing of stdin and stderr.
fetch operation which uses HttpClient
hard lock in named_lock which uses binding to port as a locking primitive:
process related API (RunnableProcess and Progress)

My current assessment is that for flush (first item) we could probably include builtin synchronous flushing API. The rest can be covered by a helper isolate + blocking synchronization primitives. I have included dcli_core/bin/sync_process.dart as an illustration of how one could run process iteractively and communicate with it synchronously. Though how exactly to refactor these APIs are better decided by the original author.

I have observed that the lion share of waitFor usage in dcli can be eliminated simply by using synchronous counterparts of dart:io asynchronous APIs. Using synchronous API also makes code considerably simpler in many places, when compared to using asynchronous counterparts and waitFor.

So my conclusion for dcli is that same as for Sass - there are no real blockers, though it would be nice to extend dart:io with synchronous flush.

Other uses

I have also used Github Code Search to search for other instances of waitFor and all that I have found is related to resolvePackageUri being async. (Plus a couple uses in code which has not been updated for 3+ years).

ntkme commented 1 year ago

There is a bit too much serialization / deserialisation going on - which is probably wasteful (especially for large messages like compilation request / responses). This can be addressed by either changing protocol slightly or by changing how we deserialise these messages (protocol buffers allow you to peak inside them based on tag numbers).

Issue for addressing this inefficiency: https://github.com/sass/sass/issues/3579

My conclusion is that there are really no blockers for Sass here.

That's fair, but I suggest to not rush setting timeline for removing waitFor. What we got so far is only a POC that works for linux, we still need to work on porting for Windows, abstract ffi logic as a package, infra and testing, etc. There are tons of todos if we decide to go down this route.

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.

mraleph commented 1 year ago

What we got so far is only a POC that works for linux, we still need to work on porting for Windows, abstract ffi logic as a package, infra and testing, etc. There are tons of todos if we decide to go down this route.

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 dart-sass-embedded tests pass there as well.

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.

The waitFor was deprecated for more than a year now and we have been talking about removing it for more than 3 years. That neither prevented people from using it, nor did it motivate Sass and DCli to migrate away from it.

My experiments demonstrate without any doubt that Dart already today provides all necessary tools for users to migrate off waitFor in a matter of days by rewriting their code. So I think it would be well within our policies to just remove waitFor in the next stable release. However, I acknowledge that these tools are fairly low-level and possibly too heavyweight for some use-cases (like Isolate.resolvePackageUri), so I am not going to propose such unpleasantly aggressive route and instead set obsoletion date to 1 year from now (3.4), which gives us all plenty of time to migrate.

nex3 commented 1 year ago

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—sass-embedded alone gets 65k downloads a week, and it's part of Google's larger strategic plans with Angular. Creating it was only possible at all because waitFor() existed. Acting like we're naughty children for using it all this time, when the Dart team wasn't putting any effort into providing an alternative, is both unhelpful and insulting. Please: work with us like we're trying to work with you.

vsmenon commented 1 year ago

@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

mraleph commented 1 year ago

@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 Mailbox has a slightly nicer API and lives together with underlying synchronisation primitives in a separate package maintained by the Dart team. My understanding, from discussions on https://github.com/dart-lang/sdk/issues/44804, is that this will be an acceptable compromise. If I missed something - please list these things.

I also encourage you to see timeline from our perspective:

The consensus within the Dart team is that we want this experimental feature removed.
We have agreed to postpone removal until we have some alternative, we have marked the feature as deprecated to encourage people to move away from it;
We have waited;
We have found an alternative;
We have made PoCs which demonstrate that alternative indeed works (e.g. for Sass it passes all tests on Windows/Linux and Mac OS X);
We have committed to package an alternative into a nice package which hides low-level details away from users which don't feel comfortable dealing with low-level code.
We have committed to a generous 1 year removal timeline.

mraleph commented 1 year ago

I want to stress one thing specifically: we were never planning to provide a complete replacement for waitFor (i.e. cover all items listed by @nex3 in https://github.com/dart-lang/sdk/issues/52121#issuecomment-1518344527) as prerequisite for its removal. waitFor (and fibers) creates the kind of async/sync interleaving, which goes against guarantees we currently provide for synchronous code. Consequently we only wanted to find is a solution (or a number of solutions) for specific use cases which would allow users currently depending on waitFor to migrate away from using it. @lrhn has explained this in his comment on the original issue. So it should come as no surprise that solutions we are coming back with don't match waitFor capabilities 1 for 1 - the only goal here is to provide a migration path for concrete uses of waitFor and not for waitFor feature in all of its generality.

bsutton commented 1 year ago

Synchronous flushing of stdin and stderr.

fetch operation which uses HttpClient

hard lock in named_lock which uses binding to port as a locking primitive:

process related API (RunnableProcess and Progress)

@mraleph thanks for the clearly significant effort you put into this.

I will have a play over the next week and see what I can merge in. This will be a little tricky as I'm currently working on breaking dcli into a set of smaller packages.

The most obvious problem is the find command where your changes step around the LimitedStreamController implementation and replace it with a simple callback. I don't believe this will work. The problem is (what I consider a flaw) in the dart stream implementation. If you have a stream writer that is fast and a stream reader that is slow you can run out of memory (calls to pause/resume do not mitigate this). Running a dcli find command over a full file system will cause the dart vm to crash.

I don't think the named lock problem should be an issue as I was already looking to move to the new File.create method that allows an exclusive lock which should be sufficient to allow the removal of the tcp port logic.

I'm also concerned about portability of the Mailbox implementation for OSX and Windows.

@pq I've reviewed the linter related issues and my original code that exhibited the problem and it now looks like the holes in the linter have be plugged.

So the summary is I'm happy to work through these issues, but I still don't see a fully workable solution.

I'm also concerned going forward that the implementation of DCli will be hampered because we don't have a full replacement for waitFor. Right now we have the dart devs attention, but in a years time when I want to do something with dcli that requires a sync version of some method I suspect that we will get little attention.

Like @nex3 I too am feeling a little bulldozered over this issue. For me the timing isn't great as OnePub is taking all my time so DCli has being taking a bit of a back seat.

mraleph commented 1 year ago

Thanks for taking a look @bsutton

The most obvious problem is the find command where your changes step around the LimitedStreamController implementation and replace it with a simple callback. I don't believe this will work. The problem is (what I consider a flaw) in the dart stream implementation. If you have a stream writer that is fast and a stream reader that is slow you can run out of memory (calls to pause/resume do not mitigate this). Running a dcli find command over a full file system will cause the dart vm to crash.

Unless I am missing something, I believe the problem you are describing can't actually happen because the producer and consumer are entirely synchronous: this means find will do nothing until callback returns, it will not produce/buffer anything. So you can run find('*', workingDirectory: '/').forEach((entry) {}) and your peak memory consumption will be dominated by nesting depth of the directory tree.

The change to the find is actually a great way to demonstrate how writing things in an entirely synchronous fashion (rather then mixing sync and async in complicated ways) greatly simplifies things. LimitedStreamController is no longer needed because everything became entirely synchronous.

I don't think the named lock problem should be an issue

Noted.

I'm also concerned about portability of the Mailbox implementation for OSX and Windows.

As noted above provided code works on OS X and Linux out of the box. I have also already ported the implementation to Windows (which was very straightforward). Just did not include the same changes in dcli fork). The entirely portable version is in dart-sass-embedded fork in this file and it builds on portable Mutex / ConditionVariable primitives which verified to work on Linux, Mac OS X and Windows. I am also fairly confident the same code will also work on Android and iOS which also expose POSIX synchronization APIs.

mraleph commented 1 year ago

@vsmenon @mit-mit ping for official approval of the breaking change - we can then start planning further steps.

mit-mit commented 1 year ago

SGTM

vsmenon commented 1 year ago

LGTM

Thanks to everyone for their engagement here! Ideally, we'd like to get folks off of dart:cli by this November to give us all another 6 months to properly assess the efficacy of alternative solutions.

Abdelazeem777 commented 1 year ago

It would be nice if we had a feature like this in flutter

mraleph commented 11 months ago

waitFor is now disabled by default on main branch and can be enabled by passing --enabled-deprecated-wait-for to the VM. We are going to fully remove waitFor in 3.4.

bsutton commented 11 months ago

I'm simply not going to be able to make this timeline for DCLI.

nex3 commented 11 months ago

The waitFor() API was disabled, but there's still no synchronous alternative to Isolate.resolvePackageUri. This has broken Dart Sass's release process without a clear avenue for a fix other than pinning ourselves to an old Dart version.

ntkme commented 11 months ago

The waitFor() API was disabled, but there's still no synchronous alternative to Isolate.resolvePackageUri. This has broken Dart Sass's release process without a clear avenue for a fix other than pinning ourselves to an old Dart version.

It is available in dev channel, but it would be tricky to use it conditionally because it is not available in stable: https://github.com/dart-lang/sdk/commit/4fddaf9486c4680ea703eef94792b421db50a1ce

nex3 commented 11 months ago

As @timsneath pointed out in https://github.com/dart-lang/sdk/issues/52121#issuecomment-1518114680, it's important to have at least one release that contains Isolate.resolvePackageUriSync() and waitFor() so that users can gracefully transition from the one to the other without being suddenly broken.

bsutton commented 11 months ago

There are also other sync functions missing.

I will try to find the list but from recollection it involves some of them close functions.

On Thu, 21 Sept 2023, 9:39 am Natalie Weizenbaum, @.***> wrote:

As @timsneath https://github.com/timsneath pointed out in #52121 (comment) https://github.com/dart-lang/sdk/issues/52121#issuecomment-1518114680, it's important to have at least one release that contains Isolate.resolvePackageUriSync() and waitFor() so that users can gracefully transition from the one to the other without being suddenly broken.

— Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/52121#issuecomment-1728558930, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32ODDHNBDYLAN245J63LX3N5EVANCNFSM6AAAAAAXGRJJN4 . You are receiving this because you were mentioned.Message ID: @.***>

bsutton commented 11 months ago

Methods that I need sync versions of:

there is a chance that these once aren't needed but more research is required.

StreamController.close(); 
StreamSubscription.close()

These once I don't see a way around.


Stdout.flush
IOSink.flush
``

mraleph commented 11 months ago

We are definitely not going to add sync methods for StreamController.close() or StreamSubscription.close and IOSink.flush - these are inherently asynchronous.

stdout.flush() does not do anything if you only use stdout.write() and similar - because stdout.write are writing things synchronously (and blocking) into the stdout. The only case when it does something useful is when you do stdout.addStream. So if you avoid addStream then you also don't need stdout.flush.

bsutton commented 11 months ago

Any chance that File.createSync(exclusive) could be changed to return true/false based on whether the file was created.

My NamedLock class currently uses a TCP socket as an interprocess locking mechanism as there is no other way to take an interprocess lock. If File.create returned true then I could use that as a locking mechanism, removing the need to use a TCP socket as a lock and then NamedLock could be synchronous (as god intended).

Alternatively sync methods for:

ServerSocket.bind
socket.close()

would allow me to use the existing TCP socket method but in a sync way.

mraleph commented 11 months ago

@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.

jimmyff commented 6 months ago

Hi, I'm using waitFor in a number of cloud build scripts. Could someone please point me in the direction of the best way to migrate this code?


    // 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

julemand101 commented 6 months ago

@jimmyff Why are you not using Process.runSync?

jimmyff commented 6 months ago

@jimmyff Why are you not using Process.runSync?

Ah, now I feel silly -that's exactly when I needed. Sorry for noise!

mraleph commented 6 months ago

The removal is complete.

tsavo-at-pieces commented 5 months ago

@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.

@mraleph I'm trying to attempt 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 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.

bsutton commented 5 months ago

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 https://github.com/dart-lang/sdk/issues/52121#issuecomment-2021893698, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OFXHVC2G2FWDCD753LY2JB4FAVCNFSM6AAAAAAXGRJJN6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRRHA4TGNRZHA . You are receiving this because you were mentioned.Message ID: @.***>

bsutton commented 5 months ago

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. The result is the code essentially deadlocks.

pseudo code:

unawaited(Isolate.spawn(_startProcess, mailbox);

mailbox.take(); // code hangs here as spawn never completes

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.

tsavo-at-pieces commented 5 months ago

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

bsutton commented 5 months ago

@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.

dart-lang / sdk

[breaking change] Remove dart:cli waitFor experiment #52121

Change Intent

Justification

Impact

Mitigation

Synchronous communication over dart:ffi

`Isolate.resolvePackageUri`

Timeline

`dart-sass-embedded`

`dcli`

Other uses