-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
waitFor
is still experimental after two years?
#39390
Comments
This function should be removed. It is fundamentally incompatible with Dart's event loop model. |
OK. If that really is the case, then can we get it marked as Leaving it around causes confusion, as evidenced by the fact that I just spent time introducing it into a CLI app I was writing. The code is cleaner than the highly-contagious |
If I recall correctly this was being used by @nex3 in SASS. |
Yes, we rely heavily on |
Is the alternative to use @rmacnak-google, how do we resolve this? |
Using Currently the library has been hidden from the public documentation by @mit-mit I don't think we currently can resolve this further at the moment: in general a lot of engineers are in favour of removing it, but I don't think we can given that @nex3 how does the code that depends on |
Describing For what it's worth, I strongly believe full fiber support on the VM would be a substantial benefit to Dart.
In JS mode, we currently use the |
I would like to add my voice to keeping waitFor and removing the experimental tag. I've just implemented a package call dshell which provides tooling and a library for building cli applications. https://pub.dev/packages/dshell dshell makes heavy use of waitFor (as in virtually every exposed function uses it). The use of waitFor greatly simplifies the dshell users experience and actually makes the library safer to use. There is also little gain in providing an async experience when writing cli scripts as there is no user expectation that the UI should be responsive when long running tasks are running. Removing waitFor would be determintal for writers of cli applications and would completely kill dshell. So I would much rather it waitFor remains :) |
Most run loops I've used can be run recursively, in order to handle situations like this. libuv is the only one I can think of that can't. CFRunLoop, Qt event loop and GMainLoop all are reentrant in order to allow synchronous callbacks. Since in dart we have no control over the loop itself, |
We might want to look at alternative APIs that involve Isolates. I think it would be less of a violation of Dart semantics to let you run async code in a separate isolate and synchronously wait for the result. |
Isolates are slow to spawn and expensive to send information across. If they're not being used for actual parallelism, insisting that they be used for synchronicity is just adding end-user pain for no value. |
For my library dcli that would not work.
Dcli aims to replace bash using dart without burdening the developer with
futures.
Making waitfor require an isolated would make dcli completely non viable as
just about every function that dcli exposes use waitfor. I suspect that the
proposed changes would make performance horrific.
Example code
```
if (!exists(somefile))
{
createDir(somedir);
touch (somefile);
'grep somedata anotherfile'.run;
}
```
Each of the above functions use waitfor.
Requiring the user to add await for each call is laborious and from
experience dangerous. The danger is that if you forget an await you can end
up causing havoc on your file system.
The original dcli library required the use of awaits and our internal team
spent half their time searching for missing awaits. waitfor makes dcli a
highly productive tool. Without it bash is probably a better solution.
From a user perspective I don't see the issue with waitfor. I understand
it has its limitations but they are not affecting the code I'm using
(except in a good way) so why are we trying to fix it.
…On Wed, 13 Jan 2021, 7:22 am Nate Bosch, ***@***.***> wrote:
We might want to look at alternative APIs that involve Isolates. I think
it would be less of a violation of Dart semantics to let you run async code
in a separate isolate and synchronously wait for the result.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#39390 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OAOUTUA7GTE3LTFQL3SZSVOXANCNFSM4JNUCWOQ>
.
|
Is there a short summary of what the current "event model" is that this feature violates? I'm guessing it may have to do with things escaping the scope of a nested loop? Thinking about this at a high level, Haskell has |
Just as an FYI, another use case for |
The "violation" is synchronous code being interrupted by asynchronous code. var sub = stream.listen(null);
callSomeMethod();
sub.onData((v) { action(); });
sub.onError((v) { handle(v); }); We generally promise that no events are sent on a subscription returned by If Similarly for futures: var future = compute():
var value = callSomeMethod();
return future.catchError((e, s) => value); Here the error on In short: Apparently synchronous code isn't actually synchronous. (This is not unique to We also break bool inLoop = false;
await for (var e in stream) {
assert(!inLoop);
inLoop = true;
callSomeMethod();
inLoop = false;
} The specification of All in all, |
This is only an invariant by fiat, though, and in practice it's quite a troublesome one. In Dart Sass, we need There are plenty of existence proofs of languages that work fine without providing this invariant. Fibers in Ruby and the It sounds like the biggest risk with |
In the end no one is forced to use waitfor.
Simply publish the risks and let those of us who need it, take those risks
if we so choose.
We have over 100k lines of code in production that aggressively uses
waitfor (via the dcli library) and the fact is that we haven't had a
problem.
If there is a path to reduce the risk then great but don't go and break our
code by removing a feature because it might break our code.
It reminds me of a discussion with a net admin I had year ago.
A denial of service bug was reported against some software we were using,
so he bought the service down so that a possible dos attack couldn't bring
the service down.
Face palm.
…On Sat, 20 Feb 2021, 7:50 am Natalie Weizenbaum, ***@***.***> wrote:
All in all, waitFor is breaking the invariant that nothing async will
happen before synchronous code has finished.
This is only an invariant by fiat, though, and in practice it's quite a
troublesome one. In Dart Sass, we need waitFor() precisely *because* we
want to call user-defined callbacks from a very large chunk of
otherwise-synchronous code that is also invoked in synchronous contexts.
There are plenty of existence proofs of languages that work fine without
providing this invariant. Fibers in Ruby and the fibers package
<https://www.npmjs.com/package/fibers> for Node.js both make it possible
to run the event loop from within a synchronous context, and they don't
break the world. Nor has waitFor() broken it, for that matter.
It sounds like the biggest risk with waitFor() is that it could be called
unexpectedly from a synchronous function whose implementation you don't
understand and wreak havoc by mutating state unexpectedly or something like
that. But that's already true—if you don't understand a function's
implementation, it could do just about anything even without calling
waitFor(). And it's doubly true in an async world with awaits sprinkled
everywhere, and it doesn't seem to cause *that much* pain there.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#39390 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OHMPXK3Y4N2IQFPC3DS73FHXANCNFSM4JNUCWOQ>
.
|
Sorry for the basic questions, but I'm really only passingly familiar with the implementation here. Am I correct in thinking that "trigger events on Is that correct? If not, what is incorrect about my (naive, high level) mental model above? If it is correct, it seems like one approach is just to say "touching asynchronous computations from a different event loop is undefined badness, don't do it"? You could even imagine trying to enforce this at runtime by attaching event loop identifiers? |
@bsutton Thanks very much for the input here, it's very valuable to know that people are using the feature and what they are using it for. |
No, no access to the subscription is needed, unless you could the reference to the subscription held by the underlying stream controller. Then it's "yes", but also always the case. Consider: var stream = someFile.openRead();
var sub = stream.listen(null);
someMethod(); // ends up calling waitFor
sub.onData(handleBytes); This call to @nex3 In that sense, Another worry I have is that it might not scale. Since the |
@lrhn What I'm trying to say is that the actual concrete effects of running async events during sync code aren't intrinsically any more dire than the effects of running unknown synchronous code. This isn't just because of sync controllers/completers, it's because the Dart code that's run in async callbacks could in theory just as easily be run synchronously through the normal call stack—state could still be manipulated in unexpected ways by a badly-behaved program. There's no guarantee that synchronous code only has local, well-documented effects.
I would certainly be in favor of moving to Ruby-style multistack |
Is that really any different from |
How about just maintaining the status quo? Keep |
I do want to see it supported.
If it's not supported then at some point it stops working.
Happy for it to be labelled with warnings but it's just too useful to not
support.
…On Tue, 23 Feb 2021 at 10:39, James D. Lin ***@***.***> wrote:
How about just maintaining the status quo? Keep waitFor, but to limit
abusing it, don't advertise it, don't recommend it, and don't support it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#39390 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OGZXLHGQSMVM2HOOMLTALTLRANCNFSM4JNUCWOQ>
.
|
Can put a slightly different perspective on this discussion. waitfor is intended only for cli applications. I don't think it's too big a stretch for me to suggest that I'm probably the most experienced end user of waitfor in the community through my work on the dcli library. Dcli is a fairly large library that exposes a large no. of functions. I've been using dcli (and waitfor) for going towards 2 years to build large no.s of console apps both for our private usage and open source projects I've released. Its perhaps a reflection of my own philosophy but my experience with building console app is that I almost never use async programming models. Async programing creates significant problems when writing console apps for no benefit. Over all of the projects I've worked on I can only remember having one problem with waitfor which was I managed to cause a deadlock. The problem was fairly easy to identify and solve. My point is that I think people are a little over concerned with the possible interactions between waitfor and async code. In my experience building console apps there is really little need to use async. In the real world the problems this thread is discussing are virtually non existent. The possible risk (which I don't believe has ever been reported) is far outweighed by the benefits. |
Sorry, I really couldn't parse this. :)
I think maybe this gets implicitly at what I'm missing. My mental model of "running a nested event loop" is that you would run with a new event and microtask queue. You seem to be assuming that this is not done (presumably because it is not in fact done in the current implementation). Is that right? So the shared mutable state is in fact the shared event loop itself? |
@leafpetersen (And makes sense you couldn't parse the sentence, some words were typoe'd.) |
@lrhn what is your take on Fibers? it allows to write synchronously looking code which can block and wait for events. It has some issues with reentrancy (e.g. an arbitrary piece of synchronous code might not be written to be "suspendable"), but otherwise it looks like a much more coherent approach than |
If "fibers" are non-concurrent operations with separate stacks, which allows each fiber to block independently (and then return to the one and only event loop), then it's power is exactly the same as If we had fibers from the start, on all platforms, it would be great. We wouldn't have streams or futures then, they're implementation artifacts of not being able to do proper blocking operations. People writing code would know that any function call can cause arbitrary things to happen (so we'd probably want to have mutexes to prevent that). Much nicer. Alas, JavaScript won't allow that, so we still need |
Right. This is why I brought up shared state. I think Haskell avoids this via parametric polymorphism - basically
Yes. The whole point is that you are now blocking right? If you block for too long... the outside world gets sad.
As best I understand, there are no guarantees whatsoever that code which is run with a timer will be run immediately after the timer expires, right? It can be blocked arbitrarily long already.
I don't know, you tell me? I think I would expect that we run until the queue is empty. Which could indeed be never. |
If the model would be "freeze the old world, run completely separate new code until it's done", then that would be hard to make work in Dart. You couldn't have any interaction with the "old world" of asynchrony, and because we have global state, that's unenforceable. That's why I'd prefer running the new code in a completely new isolate (even though it could run in the same thread as the original while that's blocked, and maybe even share the same heap, making That would be something like There'd still be the risk of the other isolate never terminating, even after the entry function has returned and completed, because of some forgotten asynchronous loop. |
@lrhn I don't see how spawnAndWait would be equivalent functionality. As I understanding an isolate runs in its own memory with no shared state. If that is correct then your proposal would break existing code and would essentially make most functions that I currently use waitFor for untenable as they often rely on shared state. I would also be concerned of the performance overhead. I've done no testing but I would assume that spawning an isolate is an expensive operation. I assume that dart has some sort of isolate worker pool but I assume the memory has to be re-initialised each time. |
Yes, |
Another use case for the functionality that |
I've just seen that waitFor had been marked as depreciate. I strongly protest this decision. waitFor is a critical function for the dcli package and general cli programming. Please reverse this unnecessary decision. No one in this thread has actually demonstrated that waitFor is broken in real world code and having used it everyday for the past two years I can tell you it works just fine. This decision is overly proscriptive and does nothing to improve the dart ecosystem and demonstratively reduces darts functionality. |
We don't have immediate plans to delete the existing implementation. We are deprecating it primarily to discourage new uses and to align the documentation with the the level of support we intend to provide for it. |
The problem with depreciation is that in two year time we are going to have
the conversation; 'waitfor' has been deprecated for two years we can delete
it now.
Depreciation should only occur if we have a replacement.
…On Wed, 29 Sep 2021, 8:26 am Nate Bosch, ***@***.***> wrote:
If you have something to replace it with then let's discuss it, until then
please leave waitFor alone.
We don't have immediate plans to delete the existing implementation. We
are deprecating it primarily to discourage new uses and to align the
documentation with the the level of support we intend to provide for it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#39390 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32ODI2UF5EDLNXO2BVZTUEI6JVANCNFSM4JNUCWOQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
This is an API which was always explicitly marked experimental and thus can be removed at any time with no replacement. Some of us are strongly in favor of doing that. The only reason why we have not is because this API currently is not a strong maintenance burden and it has a very small number of passionate users who made an unfortunate mistake of relying on an experimental API. So we weighted the cost of maintenance vs the cost of breaking these few users and decided that breaking is not worth it at this point in time. That being said this balance can shift any moment and this API can be deleted - so you are encouraged to migrate away at your earliest convenience. |
It's unfair to characterize users as "making a mistake" for using this API, whether or not it's marked as experimental. It's the only available avenue to address a substantial missing piece of functionality in Dart: the ability to write code that's polymorphic over synchrony versus asynchrony. This is clearly functionality that users need, and shaming them for taking the only option for making that happen isn't only useless, it's actively hostile to the people who use your product. If you want to ensure that people don't use this API, offer a better alternative, don't talk down to your users. |
On what metric are you assuming this is only used by a small no of users?
The vast majority of console apps are built for internal corporate usage
and I'm not aware that dart collects any API usage statistics.
Whilst dcli's user base is small it is also growing.
The typical user of waitfor is also unlikely to be even aware of this issue.
This whole conversation seems to be driven by people that aren't using the
API, pontificating on how bad it is, whilst the actual users are saying
this is critical and it works.
I don't think you are listening to the right people.
…On Wed, 29 Sep 2021, 9:02 am Vyacheslav Egorov, ***@***.***> wrote:
This is an API which was always explicitly marked *experimental* and thus
can be removed at any time with no replacement. Some of us are strongly in
favor of doing that. The only reason why we have not is because this API
currently is not a strong maintenance burden and it has a very small number
of passionate users who made an unfortunate mistake of relying on an
experimental API. So we weighted the cost of maintenance vs the cost of
breaking these few users and decided that breaking is not worth it at this
point in time.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#39390 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OA2VM4TEKY5QSC6VKLUEJCQLANCNFSM4JNUCWOQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
This informs our decision of marking it deprecated. The existing mechanism we had clearly wasn't strong enough, and marking it as deprecated should make it more obvious. We don't want the typical user to remain unaware of this issue.
This conversation is driven by the people who would be on the hook for fixing this API if we make it officially supported and it breaks. We've decided we don't want to make those promises because it carries risk and may limit our ability to change implementation details. The framing for the deprecation is not "The dart team changed their mind and decided to stop supporting this." The framing of the deprecation is "The dart team is fixing their mistake of having an unsupported API which doesn't clearly advertise the lack of support." Any discussion about the history is not intended to assign blame or make anyone feel bad about their decisions. The discussion of the history is explain that the unclear advertising of the support level is not a deciding factor that will force us into a level of support we never intended. |
This informs our decision of marking it deprecated.
I don't see how that argument holds. My experience is that most users use
an api with very little interaction with active issues unless it is causing
them a problem.
With Dcli, users won't even necessarily be aware that they are using
waitfor as its use is buried in the dcli library.
This is likely to be true of any other console apps that are published.
We've decided we don't want to make those promises because it carries a
lot of risk.
It's this analysis that appears to be flawed and goes back to my statement
about who you should be listening to.
The users of the api are saying it works just fine. It's only the
theoreticians that are saying it carries risk and I haven't seen any code
that demonstrates this in a real world scenario.
As to being on the hook the statements appear to indicate that there is
minimal maintenance burden on the code.
Going forward as things stand doesn't appear to greatly increase the risk.
I would simply ask that you leave things alone until we have a viable
alternative.
S. Brett Sutton
Noojee Contact Solutions
03 8320 8100
…On Wed, 29 Sept 2021 at 09:47, Nate Bosch ***@***.***> wrote:
The typical user of waitfor is also unlikely to be even aware of this
issue.
This informs our decision of marking it deprecated. The existing mechanism
we had clearly wasn't strong enough, and marking it as deprecated should
make it more obvious. We don't want the typical user to remain unaware of
this issue.
This whole conversation seems to be driven by people that aren't using the
API, pontificating on how bad it is, whilst the actual users are saying
this is critical and it works.
This conversation is driven by the people who *would be* on the hook for
supporting this API if we make it officially supported. We've decided we
don't want to make those promises because it carries a lot of risk.
The framing for the deprecation is not "The dart team changed their mind
and decided not stop supporting this." The framing of the deprecation is
"The dart team is fixing their mistake of having an unsupported API which
doesn't clearly advertise the lack of support."
Any discussion about the history is not intended to assign blame or make
anyone feel bad about their decisions. The discussion of the history is to
make clear that the unclear advertising of the support level is not a
deciding factor that will force us into a level of support we never
intended.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#39390 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OD5OTSJKGBN3KMGPU3UEJHXLANCNFSM4JNUCWOQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
The purpose of marking the API deprecates was precisely to leave things alone until we have a viable alternative, which is also what the documentation states. (Well, unless unexpected events change the world and our priorities in a way we haven't predicted, which is a caveat in everything.) It's now documented that we are leaving the library alone (which includes having effectively no maintenance or support), and we discourage other users from starting to use the feature (basically asking them to leave it alone too). I'm very open to suggestions for more viable alternatives. Just be aware that allowing synchronous functions to take asynchronous breaks is the problem here, not just the implementation. No matter how convenient it might be, it's fundamentally breaking the assumptions we've told people they can make about synchronous computation. (I'd expect blocking the isolate and computing the async computation in a separate isolate to be the most immediate alternative to waiting for an asynchronous computation in a synchronous function, but any other ideas are welcome too.) |
Clarify this is supported, but deprecated, and may get removed in the future should we find a suitable replacement. Bug: dart-archive/api.dart.dev#81 Bug: #39390 Change-Id: I1304a0862986ae1b365a063ee75f0041341523f5 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/214620 Commit-Queue: Michael Thomsen <mit@google.com> Reviewed-by: Lasse R.H. Nielsen <lrn@google.com>
Closing as resolved for now. We understand the concerns raised. While this isn't as clear as we -- and several ecosystem members -- would have liked, I believe it's an acceptable outcome for now: As Lasse said, we'll leave things as they are until a time where we have found a viable alternative. |
@nex3 - what's the current state of this? It appears that |
For the Node.js embedded host, we use For Dart Sass compiled to JS, we give users three options: use the Node.js embedded host instead, use the synchronous API (which isn't compatible with most build system plugins), or accept a major performance hit. We've considered the possibility of pursuing a similar |
Based on various internal discussions over the past month, I would like to move forward and remove #52121 gives the details of the proposed timeline. |
The
waitFor
function has been marked as "experimental" for two years now:sdk/sdk/lib/cli/wait_for.dart
Line 71 in ec8dcba
(see https://dart-review.googlesource.com/c/sdk/+/28920/ for its introduction)
Can we remove this line at this point? The experiment has clearly run its course. Either we need to remove this comment or we need to deprecate it (if we think it's a failed experiment for some reason).
There is value in something like this for CLI apps, of which there will be many more now that
dart2native
has landed, and I'd love us to provide reliable guidance. If there are better alternatives towaitFor
, I'd love to know about them myself!The text was updated successfully, but these errors were encountered: