-
Notifications
You must be signed in to change notification settings - Fork 106
Garbage Collection #89
Comments
No, Promises don't have timeouts.
I believe it has the same machinery as generators, e.g.: function* yo() {
var foo = 'yo';
yield foo;
// This one will never be reached
yield foo + ' dawg';
}
yo().next(); |
So to clarify, your answer is "the async function will leak memory for eternity"? |
this is the major diff (and drawback IMO) with I'm not sure I'd call that a "memory leak" so much as just un-GC'd memory. This is similar to how globals aren't GC'd (since they never go out of scope) during the lifetime of the program. This memory gets reclaimed on refresh, which in a true memory leak scenario wouldn't even be true. |
@getify thanks, that makes sense. Note that no one is saying we'll never have the ability to I got to this problem after talking to PHP internals, they're debating this behavior for generators. As far as I'm aware the spec does not allow garbage collection for: function* foo() {
try {
yield 1;
} finally {
cleanup();
}
}
(function() {
var f = foo();
f.next();
// never reference f again
})() Since that would make consumers aware of consumption behavior or finally blocks would not run. I admit I'm very rusty on the generators spec having not opened it since September. I just want to make sure we don't introduce memory leaks into the language. (And I'm wondering if we introduced ones before). |
I'm not sure, but seems like so. Some actual VM developer will give better insights: I've pointed out that we already have similar scenarios in the language. |
But it's not that all memory allocated in the async function is held forever, right? It's just the DB connection which is keeping it in memory since it's socket is registering stuff in the event loop. |
The reason I'm asking is because there are two conflicting assumptions here: that finally blocks are always run and that GC is not observable. I guess it is a programmer errors so our behavior might make sense. It aligns with C# but not with Python.
The DB connection would be held in memory forever. The code releasing it is never called. |
As I anticipated. So if there wasn't something akin to a DB connection, then that memory would be cleared regardless of whether the function resolved or not, right? Personally, I'm not too worried about this case since any situation where I had an unresolved promise like that ended up having a lot of visible effects in my applications and was caught and fixed pretty quickly. Edit: Though I could see how people used to |
This same hazard exists with |
I think it should be cleared up that |
Yes, that was my question. This is not the case in Python for example which changed this behavior for generators in 2.5 https://docs.python.org/2.5/whatsnew/pep-342.html
In C#, generators are disposable but do not dispose on GC (no finalizer). async/await is handled with synchronization context which are a completely different beast. |
Async functions have essentially identical implications on GC as generators and promises. Solving this issue is out-of-scope, although I'll note that most cancellation semantics would also make cleanup possible. Fwiw, if it's important to finalize a resource then you have to make sure you will hit the finally blocks and that means, among other things, making sure you don't have forever-pending promises. Simply wrapping the original example's second awaited promise in a timeout will fix this issue in today's semantics. |
@bterlson right, I think it's important to point out that python had to make this call and it made it differently. C# made the same call we're making. I'm not saying we should change anything in the spec but this is definitely a choice 3 programming languages made before us so it's worth mentioning. @bwoebi and @nikic are discussing it for PHP right now so they might have some useful insights. |
What change is being proposed here? This is not the place to suggest either substantial changes to the underlying promises model (ie. adding a notion of cancellation) nor are we going to entertain moving away from async functions as mostly sugar for promises. |
@bterlson I'm not proposing any changes - I'm just asking for status because I couldn't find any related discussion in the repo spec or meeting notes and this is something that has been discussed for C#, Python and now PHP. Hypothetically a possible change would be to behave like Python and allow running
Still, Python had generators behaving one way and explicitly changed it to this new behavior. PHP are considering it now. I think it was at least worth bringing up :) |
I don't understand why memory would need to be "leaked" (or "never reclaimed") here. Presumably the However, since nothing keeps a reference to the promise, the promise would get GCed. This would in turn allow the handler and its context to get GCed. So indeed, the Unless I'm missing something? |
The promise is never resolved. |
Ok, so I've thought about this a day now. We might want to introduce synchronization contexts or a similar facility in the future in order to be more aware of these things. It would be interesting and should be a TC priority IMO to find the people involved with the design of these features in Python and C# and discuss resource management with them. As for this particular issue - anything but forbidding finally would be very counterintuitive for JavaScript developers. Same goes for generators. I'll send a separate issue about generators to esdiscuss. I'm closing. @bterlson feel free to reopen if you feel discussion has not exhausted itself. Thanks/ |
I know the promise is never resolved. That doesn't change what the VM is passing as callback to the The point of my comment was that even though the promise indefinitely keeps a strong reference to the context, that doesn't prevent the context and the promise from being GCed together. |
@sicking right I agree - it disposes and |
|
@ljharb I agree - that's what I told the PHP people http://chat.stackoverflow.com/transcript/message/28781833#28781833 |
Let's say I have an async function:
And I call it:
The text was updated successfully, but these errors were encountered: