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 -> ...));
}
```
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.
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).
- 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
>
