Fix Promise memory leak

[Pauan]

As explained in the documentation, if the Promise never succeeds / fails then the callback will never be called, and it will leak memory. This pull request fixes that.

Now the done method returns a DoneHandle. When the DoneHandle is dropped it will drop the callback, thus freeing the memory (at least on the Rust side).

[koute]

What if someone actually wants the previous behavior?

In JavaScript it's pretty common to do a fire-and-forget style promises which this will make harder to do (you'll have to manually save the DoneHandle somewhere and then manually clear it when the Promise handler finishes), and it's not immediately obvious here that the promise handler will effectively cancel itself when the returned handle goes out of scope.

Wouldn't it make more sense sense to have the previous behavior as default and then allow the user to opt-in to this one?

For example like this:

    promise.done( |...| ... ); // This will stay alive until the promise is triggered.
    let handle = promise.done( |...| ... ).cancel_on_drop(); // This will cancel it when `handle` is dropped.

(or have two methods for this)

[Pauan]

No problem, just use std::mem::forget, which is also the same solution if you want to keep an EventListenerHandle or MutationObserverHandle alive forever. It's not fundamentally any different.

It would be weird to have inconsistent behavior between DoneHandle and other handle types, so if you want me to change it here then we should change it for all the handle types.

[Pauan]

If we want to make it easy to have fire-and-forget things (like event listeners, promises, mutation observers, etc.) then I think the cleanest way is to create a new Forget trait, and then implement it on the various handle types, so that way you can do this:

let handle = promise.done(|...| ...);

handle.forget();

The reason for having a .forget() method (rather than using std::mem::forget) is so that it can drop its own internal memory without running Drop on itself.

[koute]

EventListenerHandle actually doesn't drop the listener automatically when it goes out-of-scope.

And you can't use std::mem::forget to drop the handle, as that will permanently leak the references which are contained within, so will leak memory. (Which is also why I don't like the name forget, and also why I think making std::mem::forget safe was a very silly decision considering using it virtually always only makes sense when writing unsafe code, but I digress.)

The point about consistency is a good one though. I think that the behavior of not canceling on Drop is probably what we want by default. That's how things work in JavaScript; hell, even our own Fn and FnMut callbacks when passed through js! need to be dropped manually.

I can envision three use cases:

1. the user doesn't want the subscription to be ever cancelled (I think this is the most common)
2. the user wants to automatically cancel the subscription on Drop (i.e. Rust's scoping own its lifetime)
3. the user wants to manually cancel the subscription

Perhaps we should work out a convention for this? E.g. something like this (names are up for discussion):

pub trait Handle {
    type AutoCancelHandle;
    fn cancel_now(self);
    fn auto_cancel(self) -> Self::AutoCancelHandle;
}

So then:

element.add_event_listener( ... ); // Will never be dropped.
let handle = element.add_event_listener( ... ).auto_cancel(); // Will be cancelled on normal out-of-scope `Drop`.
let handle = element.add_event_listener( ... );
...
handle.cancel_now();

[Pauan]

EventListenerHandle actually doesn't drop the listener automatically when it goes out-of-scope.

That's not good, because that means it permanently leaks memory, and it's very easy to do that accidentally.

even our own Fn and FnMut callbacks when passed through js! need to be dropped manually.

I think that's a bad argument: we don't want to manually drop listeners, we must manually drop listeners because of technical limitations. If we could figure out a way to automatically drop listeners then I would suggest we use it.

Also, I don't think it's good to use implementation details to justify a public API.

I think that the behavior of not canceling on Drop is probably what we want by default.

We shouldn't make leaking memory the default behavior, especially since it's very easy to do it accidentally. That goes against Rust's overall philosophy and API design. Rust has a very clear memory model, which drastically helps to prevent memory leaks, I think we should embrace that model.

That's how things work in JavaScript

I don't think we can copy JavaScript in this situation. In JavaScript, if you add an event listener to a DOM object, you don't need to worry about removing the event listener, because the event listener will automatically be removed when the DOM object is removed from the DOM (and then the callback will be garbage collected).

That isn't the case with Rust. Even after the DOM object is removed from the DOM, even after the DOM object has been garbage collected, the Rust callback will still be consuming memory. There's just no way to avoid manual memory management, and the most convenient and Rusty way of doing manual memory management is to use the Rust idiom of tying memory to scopes, which prevents accidental memory leaks.

Perhaps we should work out a convention for this?

Yes, I agree we need a uniform way of handling this, I just disagree that leaking memory should be the default. Users should need to opt-in to memory leaks.

[koute]

Well, both approaches have its pros and cons.

For example, with Promises if we'd have cancel-on-drop by default then if you set a then callback on a Promise and you forget to call no_auto_drop() (or whatever it would be called) or forget to save it somewhere then your handler will get silently dropped, and you wouldn't even notice. (When the dropped callback is called it will throw an exception, but what if you're not testing that particular codepath and it will only trigger in production?) In case of event listeners you'd stop receiving events, and you wouldn't even know why.

I agree that preventing memory leaks is important, but that's also something that doesn't come for free. Now, I'm not saying that auto-drop-by-default is a bad idea per-se - you do make some very good points, but the situation is not purely black and white. (I'm not fully convinced myself, one way or the other.)

[Pauan]

For example, with Promises if we'd have cancel-on-drop by default then if you set a then callback on a Promise and you forget to call no_auto_drop() (or whatever it would be called) or forget to save it somewhere then your handler will get silently dropped, and you wouldn't even notice.

My plan is to add #[must_use] to the Handle types, which will give a warning if you forget to use them. Is it possible to turn that into an error instead of a warning?

When the dropped callback is called it will throw an exception, but what if you're not testing that particular codepath and it will only trigger in production?

That won't happen, I specifically tested that and made sure that it won't throw. That's handled by the done array in the code.

In case of event listeners you'd stop receiving events, and you wouldn't even know why.

Yes, but at least the programmer knows that something is wrong, so they try to figure out why it's not working, and then they fix it.

With leak-by-default the programmer doesn't even know that something is wrong, it's completely silent, so the problem never gets fixed.

Also, that issue is mostly solved by #[must_use]

---

On other issues that we disagree on, I don't have strong opinions, but I do have strong opinions about this. So I'll try to clarify my thinking as much as possible.

JavaScript programmers don't worry about memory leaks (because of the garbage collector). Rust programmers don't worry about memory leaks (because of RAII).

They have very different ways of managing memory, but in both languages the memory is automatically managed. In other words, the default is to not leak memory. This gives a huge amount of peace-of-mind to the programmer, and is very important.

If we leak memory by default in stdweb, this is contrary to the intuition and expectations of both Rust and JavaScript programmers:

• Rust programmers will expect RAII. They aren't used to manually dropping things. This causes annoyance.

• JavaScript programmers will use JavaScript idioms (such as creating fire-and-forget event listeners).

  Their code will compile correctly, it will even run correctly at runtime, but unlike the equivalent JavaScript code, it has a silent memory leak.

  It is very easy to accidentally leak memory, because JavaScript idioms which don't leak memory in JavaScript do leak memory in stdweb.

  Even worse, there is absolutely no indication whatsoever that it's leaking memory. No warning, no panic, nothing at all.

  For simple examples like todomvc this might not seem like a big deal, but this will bite programmers who are working on even semi-complicated web apps.

  It's common to dynamically create DOM nodes and add event listeners, and every time you add an event listener it will leak a little bit more memory.

  With hundreds of DOM nodes being created all the time, this will quickly create a big memory leak. (Single Page Application web sites do create/destroy hundreds of DOM nodes all the time, so this isn't a hypothetical example).

The end result is that leaking memory by default will cause very subtle and hard to find bugs, so stdweb will get a reputation for being unreliable, buggy, and causing memory bloat. Or at the very least it will get a reputation for being hard-to-use and easy-to-screw-up.

Programmers of both Rust and JavaScript do not expect silent-accidental-permanent-memory-leaks-by-default-with-no-warning. Requiring the programmer to be careful and use discipline to avoid memory leaks isn't a good solution.

I think that leaking by default is contrary to both the JavaScript and Rust principles, and it's also contrary to stdweb's principles (which is to provide safe Rusty APIs for JavaScript).

Yes it is true that js! requires you to manually drop callbacks, but that's an implementation detail. When creating a JavaScript-to-Rust API you do need to worry about memory leaks, but programmers who are using your API should not need to worry about memory leaks. It's the same principle as unsafe: you need to be very careful when using unsafe, but you should wrap it in a safe API.

Given all of the above, my proposed solution is the following:

1. Create a Leak trait (exact name to be decided, but I like Leak):

   trait Leak {
       fn leak( self );
   }

   We don't want to use std::mem::forget because:

   • Using .leak() is more convenient.
   • .leak() is an official and documented API, as opposed to std::mem::forget which is considered technically-safe-but-a-bit-gross.
   • The name leak is much more intuitive than forget.
   • Using .leak() allows the Handle to clean up its internal memory.

2. Add #[must_use] to all of the Handle types.

3. Implement Leak for all of the Handle types.

The end result is the following:

// This gives a warning because of #[must_use].
//
// If the programmer ignores the warning, then it won't
// leak memory, but it probably won't behave how the
// programmer wants, which causes the programmer
// to investigate why it isn't working, and then fix it.
element.add_event_listener(...);

// This makes it clear that it is *intentionally* leaking
// memory, with all the pros and cons of doing so.
element.add_event_listener(...).leak();

// This makes it clear that the handle is now block-scoped
// and will be cleaned up when the scope ends.
let handle = element.add_event_listener(...);

// We can use normal Rust idioms to drop early.
drop(handle);

Let's compare that with the leak-by-default model:

// No warning whatsoever that it is leaking memory. The
// programmer doesn't even realize anything is wrong, so
// it doesn't get fixed.
//
// In addition, there is no way of making it clear to the compiler
// or other programmers that it is intentionally leaking: it might
// be leaking intentionally, *or* it might be leaking accidentally.
// We have no way of knowing which it is.
element.add_event_listener(...);

// This makes it clear that the handle is now block-scoped
// and will be cleaned up when the scope ends.
let handle = element.add_event_listener(...).auto_drop();

// We can use normal Rust idioms to drop early.
drop(handle);

[Pauan]

Is it possible to turn that into an error instead of a warning?

It seems it is possible:

#![deny(unused_must_use)]

But I think that will only affect the current crate, not other crates which are using stdweb.

[Pauan]

Also, I want to clarify one more thing:

the user doesn't want the subscription to be ever cancelled (I think this is the most common)

I don't think it is the most common.

Web apps are constantly creating new DOM nodes and destroying old DOM nodes. When you destroy the old DOM nodes you must cancel the event listeners (to free up memory and avoid a memory leak).

Even the todomvc example leaks (a lot of) memory, because it doesn't correctly cancel the event listeners when the DOM is destroyed. We should fix it.

If you have a DOM node which lives for the entire duration of your app, and it contains an event listener which also lives for the entire duration of your app, then you don't need to worry about cancelling the event listener.

That situation does happen, but in every other situation you do need to worry about cancellation. And I think it is more common to dynamically create/destroy DOM nodes, especially with the recent craze of virtual DOM and SPA.

[Pauan]

P.S. Rather than using #[must_use], I think we should use this:

#[must_use = "The event listener will be cancelled when the handle is dropped. Use .leak() if you want the event listener to never be cancelled (this will leak memory!)"]

In addition to giving a warning, it also explains to the user what the problem is, and one potential way how they can fix it.

[koute]

Thanks for the lenghtly explanation!

Hmm... yeah, mostly sounds good to me!

Although I'm thinking... is there any way we could wrestle back some ergonomy after we'd make the handles cancel-on-drop by default? For example, for event listeners besides the leak method could we add something like cancel_on_unmount() (there is probably a better name for this) which would automatically drop the closure when a given element is removed from the DOM? (This would obviously work only for event listeners which can be inserted into the DOM.)

[Pauan]

@koute I'm not sure if that's possible, since the only way to be notified when a DOM node is removed is to use MutationObserver, and I'm not sure how practical / feasible that is (in addition to concerns about performance).

I think a better approach is to encourage a Virtual DOM style approach, which internally manages everything and automatically frees the callbacks when the DOM nodes are removed. I am currently working on prototyping something like that, I'll show it off soon.

[spease]

I think the confusion here is from unclear ownership.

When I see “add_event_listener” as a method, I assume that the object is taking ownership of the listener (eg like adding an item to a vector). If destroying the DOM node doesn’t clean up the event listeners for it, I’d be totally oblivious to it until things started breaking.

If the handle is actually what represents ownership of the listener, then I agree with the must_use and default drop behavior. I’d also wonder if an alternative style would be better - maybe a rename like create_event_listener or EventListener::for(node). Though the latter just seems too awkward to be discoverable or practical.

In general I’d note that if people wanted to write javascript that behaved like JavaScript they’d just use a js! block - the Rust code should behave like Rust code even if there’s some deviation from JavaScript. If naively using it like js would lead to bad things happening, it should be gated in some way.

Btw I think “persist” would be better than “leak” or “forget” unless this really is something that people should not be doing.

[Pauan]

@spease In general programmers shouldn't leak memory unless they've carefully considered the benefits, drawbacks, and alternatives of doing so.

Using .leak() is something which should generally only be done at the top level, when you're sure that the DOM nodes won't be removed. I would personally frown upon any other uses of .leak()

Of course there's nothing stopping people from creating leaks in other situations, but that's their choice. I just want them to know that they are making that choice.

[Pauan]

@koute I added in a new Cancel trait (similar to Drop), and a new AutoCancel struct.

After spending a lot of time thinking about this (and trying out different implementations), I think this is the cleanest, simplest, most flexible, and fastest approach.

Please take a look and let me know if you think Cancel + AutoCancel is good. If it's okay, I will then add in documentation, unit tests, etc.

P.S. Since this is a breaking change, it will have to be delayed to version 0.5.0. Have you considered having two branches: master and next, with master containing non-breaking changes and next containing breaking changes?

[koute]

Hmm... I like the general design of it! Having a wrapper type which would wrap the original handle certainly feels very clean.

Although I think I'd prefer the name CancelOnDrop instead of AutoCancel to be more explicit, so if you could change it then it'd be great. (:

As far as another branch goes, hm... that's definitely something to consider, although rebasing that might be a pain. I'll think about it.

For now could you split your changes so that we could merge only the trait itself + changes to the Promise APIs? I do want this applied consistently, but for now we can apply the changes only to Promise and still release that in the 0.4 series. (I've feature-gated the Promise before releasing 0.4 so I don't consider any changes to it breaking.)

[spease]

    /// This method returns a [`DoneHandle`](struct.DoneHandle.html). When the [`DoneHandle`](struct.DoneHandle.html)
    /// is cancelled it will drop the `callback` and the `callback` will never be called. This is useful if you are
    /// no longer interested in the `Promise`'s result.
    ///
    /// But if you *are* interested in the `Promise`'s result, then you either need to make sure to keep the handle
    /// alive until after the callback is called, or you need to use the [`leak`](struct.AutoCancel.html#method.leak) method.

I'd understand something like this better:

Ownership of the listener is retained by the DoneHandle by default. This means you should generally retain ownership of the DoneHandle or the Promise derived from it for as long as you want the listener to function. Once the DoneHandle or Promise is dropped, the listener will be destroyed and stop working.

If you cannot retain ownership of the DoneHandle or the Promise, you can call the leak() method to consume the Promise without dropping the listener. Use leak() with care, as the listener will continue to use memory until the application is terminated, even if the DOM object it is listening to gets deleted.

But feel free to ignore this if you feel it's more confusing or if I didn't understand what's going on correctly.

[Pauan]

@spease Thanks for the suggestion, I'm always open for improving the wording to make it easier to understand.

However, these parts seem a bit odd to me:

This means you should generally retain ownership of the DoneHandle or the Promise derived from it for as long as you want the listener to function.

The listener is owned exclusively by the DoneHandle, so once the DoneHandle is cancelled the listener will be cleaned up, regardless of whether the Promise is still alive or not.

Also, the Promise isn't derived from the DoneHandle, it's the other way around.

If you cannot retain ownership of the DoneHandle or the Promise, you can call the leak() method to consume the Promise without dropping the listener.

Dropping the Promise does not clean up the listener, it can only be cleaned up by cancelling the DoneHandle.

If it were possible to clean up the listener when the Promise is dropped then it would not be necessary to use DoneHandle at all. But it's not possible[1], which is why DoneHandle exists.

Also, calling .leak() only consumes the CancelOnDrop, it does not consume the DoneHandle or the Promise

---

• [1] Technically we could create a wrapper around Promise which would contain a Vec<DropHandle>, and it would then cancel all of the DropHandles when the Promise wrapper is dropped.

  But that's quite a lot more complicated (and slower) than our current DropHandle implementation (which is very fast and lightweight).

[Pauan]

@koute I made your suggested changes, but this isn't ready to merge yet: I still need to add in documentation and unit tests.

[Pauan]

@koute It turned out that Cancel is useful for other crates too (such as my signals crate), so I decided to make a discard crate.

I changed this pull request to use the discard crate. I think this is ready to merge, assuming you think the changes are okay.

---

@spease I changed the documentation for Promise::done, if you have some spare time could you please look it over and let me know if you think it is easier to understand?

[spease]

@Pauan Yes, that makes the ownership situation clear, thanks

[Pauan]

@spease Great, glad to hear it!

[koute]

Thanks for all the great work!

Hmmm... however, I think we might have jumped the gun here a little with abstracting the trait away to a separate crate. Don't get me wrong - having something like that could be useful in general, however in the context of stdweb itself I can see a few issues with it:

1. The name - discard - makes perfect sense in a generic context, however I think that here maybe cancel would be better? It would make it more obvious what it does. (e.g. "cancel a timeout" sounds better to me than "discard a timeout")
2. The leak method not taking &self makes perfect sense in a generic context, but here that's just an unnecessary hit to ergonomics. (e.g., especially in Promise-heavy code where you might often want this behavior)
3. The name leak is, I think, misleading, as calling it will not always result in a leak (e.g. in case of Promises if the handler gets called it will not result in a leak.). I'd like to come up with a more appropriate name here, although right now I don't have anything in mind in particular.
4. We might want to implement some extra traits on these types in the future, and having it in a separate crate will make it impossible due to Rust's coherence rules.

And in general ignoring the issues I've mentioned I think this area is still sufficiently underexplored that it's too early to commit to any general-purpose, abstract mechanism.

[Pauan]

@koute The name - discard - makes perfect sense in a generic context, however I think that here maybe cancel would be better? It would make it more obvious what it does. (e.g. "cancel a timeout" sounds better to me than "discard a timeout")

But you're not cancelling the Promise (because Promises cannot be cancelled), you're disposing of the memory for the callback.

I tried (literally) dozens of names, and I decided on discard because I think it is better than cancel.

It's best to think of Discard as being exactly the same as Drop, except it can be turned off.

So you're not cancelling a Timeout, you're dropping a Timeout (which has the indirect side-effect of cancelling the timeout, but that will vary from type to type).

In other words, wherever you would implement Drop on a type you can instead implement Discard. Saying "it discards the Timeout" isn't any weirder than saying "it drops the Timeout".

The leak method not taking &self makes perfect sense in a generic context, but here that's just an unnecessary hit to ergonomics. (e.g., especially in Promise-heavy code where you might often want this behavior)

What do you mean? The old version also took self. Do you have any examples of code which would benefit from it taking &mut self?

The name leak is, I think, misleading, as calling it will not always result in a leak (e.g. in case of Promises if the handler gets called it will not result in a leak.). I'd like to come up with a more appropriate name here, although right now I don't have anything in mind in particular.

I like the name because it suggests "this is dangerous, be careful when using this!" Which is true. Even in the case of Promises, you must carefully guarantee that the Promise will eventually succeed / fail.

You can easily guarantee that for Promises you have created, but it is a lot more work to guarantee that for Promises you have not created. So the name is an extra reminder that "this can leak memory! You have to put in extra work to prevent that!"

And in the case of event listeners, requestAnimationFrame, etc. it will always leak memory, so Promise::done is a bit of a special case.

We might want to implement some extra traits on these types in the future, and having it in a separate crate will make it impossible due to Rust's coherence rules.

Why would that be a problem? DiscardOnDrop implements Deref and DerefMut, so it will pass through all methods to the underlying type, so there shouldn't be any issue with coherence rules.

And in general ignoring the issues I've mentioned I think this area is still sufficiently underexplored that it's too early to commit to any general-purpose, abstract mechanism.

Why is it better to explore this space in the stdweb crate and not the discard crate?

[koute]

But you're not cancelling the Promise (because Promises cannot be cancelled), you're disposing of the memory for the callback.

I'd say it's the other way around - you're canceling the subscription you've made; disposing of the memory is just a side effect. (: That's how RAII is treated in general - e.g. even in C++ (which is the poster child for RAII-style resource handling) the handles are never purely about memory but about ownership of a given resource which might have nothing to do with memory management at all.

I think it's more useful to think in terms of "will this stay alive?" rather than "will this leak?". The memory leak itself is a secondary effect - the leak is there because a given object/subscription is still around.

So you're not cancelling a Timeout, you're dropping a Timeout (which has the indirect side-effect of cancelling the timeout, but that will vary from type to type).

Again, isn't it the other way around? (:

You call set_timeout, and it returns you a handle. That handle controls the ownership, the lifetime of that timeout. If it's dropped the timeout is cancelled. But you can relinquish the ownership of it, in which case it will not be cancelled when your handle to it goes out of scope. In this case (and in e.g. case of requestAnimationFrame) relinquishing the ownership has nothing to do with memory as the memory will always be freed automatically regardless of what you do.

It's best to think of Discard as being exactly the same as Drop, except it can be turned off.

Well, if it were exactly the same as Drop then we could use mem::forget (since that's how you turn off Drop), but the point is that it isn't. (: Calling mem::forget will always leak memory. Calling the method from this trait will only sometimes do. setTimeout, requestAnimationFrame, setInterval and MutationObserver will never leak**, Promise::done may sometimes leak, add_event_listener will always leak for non-transient nodes.

** - where by "leak" I mean memory which is wasted; e.g. if you have a setInterval running then that memory is not really leaked as long as that setInterval is active

I like the name because it suggests "this is dangerous, be careful when using this!" Which is true.

But it's incorrect because calling it will not always result in a leak. (: I think that having a good must_use message along with a fat, bold warning in the docs for cases it can leak would suffice as a warning.

What do you mean? The old version also took self. Do you have any examples of code which would benefit from it taking &mut self?

Sorry, the & was there by mistake. I meant that you can't just call .leak() - you have to call DiscardOnDrop::leak(...).

Why would that be a problem? DiscardOnDrop implements Deref and DerefMut, so it will pass through all methods to the underlying type, so there shouldn't be any issue with coherence rules.

Deref and DerefMut work only in very specific circumstances (when you use the dot, and when you make a deref coercion). You can't, for example, do impl Foo for Bar {} and then pass that to an fn baz<T: Foo>(v: T) if you wrap your value - that would need an impl for the wrapper itself.

Why is it better to explore this space in the stdweb crate and not the discard crate?

For quite a few reasons:

1. dealing with multiple crates is always more awkward,
2. as previously mentioned Rust's coherence rules prevent some uses which we may or may not want,
3. keeping it inside stdweb we can have something which is less generic and more specific to our uses (e.g. we could document in must_use in which cases it could actually leak memory),
4. we don't have to make everything public (e.g. if we add something experimental or internal); if you have multiple crates then we can't really do that,
5. your discard crate is already marked as 1.0 (:

I have nothing against your discard crate - I think in a generic context it'd be great! For stdweb I just explicitly want something localized.

[Pauan]

@koute I'd say it's the other way around - you're canceling the subscription you've made; disposing of the memory is just a side effect. (: That's how RAII is treated in general - e.g. even in C++ (which is the poster child for RAII-style resource handling) the handles are never purely about memory but about ownership of a given resource which might have nothing to do with memory management at all.

Sure that's how RAII is in general, but with Promises it's not cancelling the subscription, it's only cleaning up memory and nothing else.

RAII serves the dual purpose of managing resources and managing memory (which is a specific type of resource), that's why Rust calls it Drop and not Cancel

We were discussing the name "discard" vs "cancel", and I said that "it discards the Timeout" isn't any weirder than saying "it drops the Timeout", therefore the name "discard" is fine.

In addition, because there are some types (e.g. DoneHandle) which are not cancelled (they are used solely for memory cleanup), "discard" makes more sense than "cancel".

** - where by "leak" I mean memory which is wasted; e.g. if you have a setInterval running then that memory is not really leaked as long as that setInterval is active

But after the setInterval or whatever is no longer wanted (by the programmer) then it will be leaking memory.

So the only time you can say that it's not leaking is if the programmer wants it to last for the entire duration of their program, which does happen, but it's not the norm.

But it's incorrect because calling it will not always result in a leak. (: I think that having a good must_use message along with a fat, bold warning in the docs for cases it can leak would suffice as a warning.

I don't think it's incorrect, since the overwhelming majority of the time it will leak. And even the mere possibility of leaking is enough justification for the programmer to be more careful.

In any case, do you have a suggestion for a better name?

Sorry, the & was there by mistake. I meant that you can't just call .leak() - you have to call DiscardOnDrop::leak(...).

That was intentional. I implemented leak as a function for the same reason that Rc uses functions (and not methods): the DiscardOnDrop struct implements Deref and DerefMut, so it delegates methods to the underlying type.

If I implemented leak as a method, what if the type also has a method called leak? Then you can't use it (or at least not easily).

Deref and DerefMut work only in very specific circumstances (when you use the dot, and when you make a deref coercion).

Interesting, I didn't know that. It's still possible to leak the value to use the underlying traits, but I agree that's not very good.

However, the alternative APIs I tried had other issues, so I still think this is the best API, even with the trait issue. And I would advocate for the same API regardless of whether it's in an external discard crate or not.

[Pauan]

Also, to address some more points:

dealing with multiple crates is always more awkward,

I agree, though stdweb can always re-export DiscardOnDrop and Discard

keeping it inside stdweb we can have something which is less generic and more specific to our uses (e.g. we could document in must_use in which cases it could actually leak memory),

I would be very surprised if stdweb documented that in the must_use!

That should be documented in the individual types/methods (e.g. the DoneHandle type or Promise::done method).

we don't have to make everything public (e.g. if we add something experimental or internal); if you have multiple crates then we can't really do that,

What parts would you want to make private? The API for the discard crate is incredibly small, there's nothing really that could be made private.

your discard crate is already marked as 1.0 (:

Yup, that was an intentional choice, since after careful consideration I think the discard crate is the right API, and it's so small and simple that it's very unlikely to change.

But if a major issue came up I would be fine with making version 2.0. The pain should be small because not very many crates are using the discard crate.

[koute]

Sure that's how RAII is in general, but with Promises it's not cancelling the subscription, it's only cleaning up memory and nothing else.

It's not only cleaning up memory, it's also cancelling the subscription you've made. The Promise itself may still live in the JS environment, but what do you think will happen if it's triggered after you drop the subscription handle? Yes, nothing will happen. Since you've cancelled the subscription. That isn't "only cleaning up memory", no? (:

But after the setInterval or whatever is no longer wanted (by the programmer) then it will be leaking memory.

But, again, that's not really a memory leak. (: If you call setInterval it is expected that it will run in perpetuity until it is canceled. If you tell your alarm clock to wake you up every morning at 6 AM would you consider that a memory leak? Of course not! You could care less that it has to allocate memory to remember to wake you up - what's important is that it's still running (and doing visible work) because you still haven't canceled it. It's not leaking memory because that memory is not lost - it's actively used to do the work you asked it to do! And once you cancel that work the memory will be also released. So in this case the fact that any memory is being managed is entirely irrelevant.

I don't think it's incorrect, since the overwhelming majority of the time it will leak.

Again, in my previous post I've enumerated in which cases it will actually leak memory, and if I can count correctly there are more unique cases where it won't leak than it will. (: I understand the need to signal to the user that they have to watch out, but I'd rather not tell them that something will leak, and then in a tiny fine print on the bottom of the page say that I lied and it only leaks in certain cases.

In any case, do you have a suggestion for a better name?

Hmm... well, naming is hard, you know. (: release_ownership? (Although maybe that's a little long?) Maybe even forget? (This has the obvious problem that it's semantics are different than mem::forget yet it has a similar name.)

That was intentional. I implemented leak as a function for the same reason that Rc uses functions (and not methods): the DiscardOnDrop struct implements Deref and DerefMut, so it delegates methods to the underlying type.

Yes, I'm perfectly aware of this.

If I implemented leak as a method, what if the type also has a method called leak? Then you can't use it (or at least not easily).

Yes. But I don't care about this in stdweb, since I don't plan for any of our types to have a leak method, and calling it through its fully qualified form will get annoying very fast, especially when calling methods which take callbacks. (This is one of the points I was trying to make - by deliberately going for a non-generic solution we don't have to worry about this shadowing a legitimate method on the underlying type.)

I would be very surprised if stdweb documented that in the must_use!

If there aren't many that cases of when it could actually leak memory - why not? (Something like "for Foo::bar and Baz::bloop calling .leak() will leak memory") That said, in the future this will probably be a moot point once rust-lang/rust#43302 stabilizes. (So we could then add a specialized #[must_use] to every function.)

What parts would you want to make private? The API for the discard crate is incredibly small, there's nothing really that could be made private.

Right now - none, because as you've said the API is pretty small. But that doesn't mean that we won't want to add something there in the future.

[koute]

@Pauan Anyhow, even though we don't have a consensus here I'd like to move this forward and get it merged. We can leave the name bike-shedding for later; for now could you just copy-paste the contents of your discard crate into stdweb and change the leak method to take self?

[Pauan]

@koute Sorry, I've been busy with other things. I was planning on replying to you (and also making your suggested changes). Is this time-sensitive?

[Pauan]

Okay, so I finally had a chance to get this fixed.

I created a new DiscardOnDrop struct, which has a leak method (as you requested).

However, I did not create a new Discard trait: it's using the Discard trait from the discard crate. The reason is because there are things (such as my dominator library) which are generic over the Discard trait, so using two traits would break that.

In practice, what that means is that if users want to create new types and then implement Discard for their types then they need to use the discard crate.

But if they simply want to use the DiscardOnDrop struct (which is defined in stdweb) then they don't need to worry about the Discard trait.

[koute]

Hmmmm... okay, as a compromise that sounds fine for now.

I might define my own trait in the future though. (If I'll need to fiddle around with it in an incompatible way, for example.) That said, even if I would do that I'd be open to having an optional non-default feature which would basically do impl YourDiscard for T where T: MyDiscard {} if possible, which would make your discard a superset of mine so it would hopefully alleviate any compatibility issues in cases where someone would want to use it in a generic context.

Oh and I reserve the right to rename stuff, although I'll have to think about this further. (Naming stuff is hard.) (:

Thanks!

[Pauan]

Earlier I had mentioned creating a virtual DOM library for stdweb, and I have finally published it to crates.io. You can use it like this:

html!("div", {
    event(|e: ClickEvent| {
        ...
    });
    children(&mut [
        html!("div", {
            style("width", "50px");
            style("height", "50px");
            style("background-color", "green");
        }),
    ]);
})

It will automatically clean up things like event listeners when the DOM node is removed, and it has a ton of other useful stuff as well.

You can read more about it here.