On 15/08/2025 12:39, Johan Vos wrote:
> Adding to this, I investigated what was exactly happening in my case.
> There was a Subscription with a few thousands recursive subscriptions,
> which were created by `and`.
> However, only a few of those subscriptions were still "active".
> When calling Subscription.unsubscribe() in case of a combined
> subscription, the listeners will be removed, but the inner
> Subscription objects are not removed, so the combined subscription
> keeps having multiple inner subscriptions.
> This is a different, but related issue. The following pattern is thus
> wrong and dangerous:
> ```
> Subscription s = Subscription.EMPTY;
> for (int i = 0; i < 100; i++) {
> s.unsubscribe();
> s = s.and(fooProperty.subscribe(val -> ...));
> s = s.and(barProperty.subscribe(val -> ...));
> }
> ```
Yeah, this pattern is not correct use, you need to reset `s` back to
Subscription.EMPTY or just overwrite it. I have been working with
Subscriptions for a while, and I was thinking it might be really great
if `s.unsubscribe()` returned the empty Subscription, because then you
can write it as:
s = s.unsubscribe();
Or even:
s = s.unsubscribe().and(fooProperty.subscribe( ... );
I was hesitant to suggest it, but if others feel the same way, we could
consider it. Now that I've used subscriptions a bit more in projects, I
find that returning Subscription.EMPTY could be a huge help.
>
> The `s.unsubscribe` keeps strong references to all previously added
> subscriptions. It has to recursively loop over a growing list of
> Subscription.unsubscribe() calls.
> Adding `s = Subscription.EMPTY` after `s.unsubscribe()` fixes that
> problem.
Indeed :)
I was already wondering how you got into the stack overflow situation,
as you'd need quite a lot of subscriptions. I sometimes however
subscribe entire models (consisting of 50+ properties), but even then it
would be hard to get a stack overflow, so I was imagining some huge
model or list of properties where each individual property needed a
chained subscription.
Perhaps the doc could mention that `and` chaining is (even after the
fix) intended for low numbers, while `combine` is more suitable for high
numbers of subscriptions.
>
> So I believe there are 2 different issues:
> 1. as reported in the original post, the recursive approach in
> unsubscribe can lead to StackOverflowErrors in case of many chained
> subscriptions
> 2. it is not clear from the documentation that
> Subscription.unsubscribe() is not removing the subscriptions (it just
> unsubscribes, but keeps the whole structure on the heap).
We can clarify this better, or perhaps it already becomes much more
clear if it returned the empty subscription. The disconnect here is I
think that people may reason that a Subscription is something that can
be altered and re-used, but in reality it is immutable, and so
`unsubscribe` can't and won't modify it. It doesn't even know the status
of the subscription (still active, or was it unsubscribed already).
The simplicity adds to its power, as you can basically make anything
into a Subscription (it's basically a Runnable), so you can do:
Subscription s = fooProperty.subscribe( ... )
.and(() -> log.info("unsubscribe was called")) // log something
.and(() -> file.close()) // close resources (but be careful of
exceptions, and it must be idempotent)
.and(() -> control.removeEventHandler( ... )); // remove an
event handler, even though it doesn't support subscriptions
Or with combine:
Subscription s = Subscription.combine(
fooProperty.subscribe(...),
() -> log.info(),
() -> file.close(),
() -> control.removeEventHandler( ... )
);
And then unsubscribe basically runs all your clean-up code in one go.
--John
>
> - Johan
>
>
> On Wed, Aug 13, 2025 at 10:12 PM John Hendrikx
> <john.hendr...@gmail.com> wrote:
>
> You're right, this should preferably not happen. The
> implementation is
> simple, but it does limit how many subscriptions you can chain.
>
> The `combine` variant does not have the same issue.
>
> I can rewrite it to avoid recursion, but it needs to be done very
> carefully as the subscriptions chained with `and` basically form a
> graph, and unsubscribing an older reference should not unsubscribe
> more
> than it would have done before calling `and`.
>
> --John
>
> On 13/08/2025 12:06, Johan Vos wrote:
> > Hi,
> >
> > The current implementation of Subscription.unsubscribe() uses
> > recursion to unsubscribe the chain of subscriptions. This can
> lead to
> > a StackOverflowError in case there are many chained subscriptions.
> > Running the following code demonstrates this:
> >
> > ```
> > void testSubs() {
> > SimpleStringProperty prop = new
> SimpleStringProperty("simpel");
> > Subscription s = prop.subscribe(() -> {});
> > for (int i = 0; i < 100000; i++) {
> > Subscription t = prop.subscribe(() -> {});
> > s = s.and(t);
> > }
> > s.unsubscribe();
> > }
> > ```
> >
> > This results in
> > ```
> > java.lang.StackOverflowError
> > at
> >
>
> javafx.base@26-ea/javafx.util.Subscription.lambda$and$0(Subscription.java:103)
> > at
> >
>
> javafx.base@26-ea/javafx.util.Subscription.lambda$and$0(Subscription.java:103)
> > at
> >
>
> javafx.base@26-ea/javafx.util.Subscription.lambda$and$0(Subscription.java:103)
> > ...
> > ```
> >
> > While it's unusual (and in most cases a very bad idea) to chain that
> > many Subscriptions, I don't think this should give a StackOverflow
> > Error. I believe it is better to avoid recursion in the
> > implementation. If people agree, I'll file an issue for this.
> >
> > - Johan
>
