<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p><br>
</p>
<div class="moz-cite-prefix">On 15/08/2025 12:39, Johan Vos wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CABxFH2Fajhv5a=wdXeTQmr69s4PPL25xBtB+ynWu3NKw3MWpcA@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">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`.
<div>However, only a few of those subscriptions were still
"active".</div>
<div>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.</div>
<div>This is a different, but related issue. The following
pattern is thus wrong and dangerous:</div>
<div>```</div>
<div>Subscription s = Subscription.EMPTY; </div>
<div>for (int i = 0; i < 100; i++) { </div>
<div> s.unsubscribe();</div>
<div> s = s.and(fooProperty.subscribe(val -> ...));</div>
<div> s = s.and(barProperty.subscribe(val -> ...));</div>
<div> }</div>
<div>```</div>
</div>
</blockquote>
<p>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:</p>
<p> s = s.unsubscribe();<br>
<br>
Or even:<br>
<br>
s = s.unsubscribe().and(fooProperty.subscribe( ... );<br>
<br>
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.<br>
</p>
<blockquote type="cite"
cite="mid:CABxFH2Fajhv5a=wdXeTQmr69s4PPL25xBtB+ynWu3NKw3MWpcA@mail.gmail.com">
<div dir="ltr">
<div><br>
</div>
<div>The `s.unsubscribe` keeps strong references to all
previously added subscriptions. It has to recursively loop
over a growing list of Subscription.unsubscribe() calls.</div>
<div>Adding `s = Subscription.EMPTY` after `s.unsubscribe()`
fixes that problem.</div>
</div>
</blockquote>
<p>Indeed :)</p>
<p>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.</p>
<p>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.<br>
</p>
<blockquote type="cite"
cite="mid:CABxFH2Fajhv5a=wdXeTQmr69s4PPL25xBtB+ynWu3NKw3MWpcA@mail.gmail.com">
<div dir="ltr">
<div><br>
</div>
<div>So I believe there are 2 different issues:</div>
<div>1. as reported in the original post, the recursive approach
in unsubscribe can lead to StackOverflowErrors in case of many
chained subscriptions</div>
<div>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).</div>
</div>
</blockquote>
<p>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).</p>
<p>The simplicity adds to its power, as you can basically make
anything into a Subscription (it's basically a Runnable), so you
can do:</p>
<p> Subscription s = fooProperty.subscribe( ... )<br>
.and(() -> log.info("unsubscribe was called")) // log
something<br>
.and(() -> file.close()) // close resources (but be
careful of exceptions, and it must be idempotent)<br>
.and(() -> control.removeEventHandler( ... )); //
remove an event handler, even though it doesn't support
subscriptions<br>
<br>
Or with combine:<br>
<br>
Subscription s = Subscription.combine(<br>
fooProperty.subscribe(...),<br>
() -> log.info(),<br>
() -> file.close(),<br>
() -> control.removeEventHandler( ... )<br>
);<br>
<br>
And then unsubscribe basically runs all your clean-up code in one
go.</p>
<p>--John<br>
</p>
<blockquote type="cite"
cite="mid:CABxFH2Fajhv5a=wdXeTQmr69s4PPL25xBtB+ynWu3NKw3MWpcA@mail.gmail.com">
<div dir="ltr">
<div><br>
</div>
<div>- Johan</div>
<div><br>
</div>
</div>
<br>
<div class="gmail_quote gmail_quote_container">
<div dir="ltr" class="gmail_attr">On Wed, Aug 13, 2025 at
10:12 PM John Hendrikx <<a
href="mailto:john.hendrikx@gmail.com" moz-do-not-send="true"
class="moz-txt-link-freetext">john.hendrikx@gmail.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">You're
right, this should preferably not happen. The implementation
is<br>
simple, but it does limit how many subscriptions you can
chain.<br>
<br>
The `combine` variant does not have the same issue.<br>
<br>
I can rewrite it to avoid recursion, but it needs to be done
very<br>
carefully as the subscriptions chained with `and` basically
form a<br>
graph, and unsubscribing an older reference should not
unsubscribe more<br>
than it would have done before calling `and`.<br>
<br>
--John<br>
<br>
On 13/08/2025 12:06, Johan Vos wrote:<br>
> Hi,<br>
><br>
> The current implementation of Subscription.unsubscribe()
uses<br>
> recursion to unsubscribe the chain of subscriptions. This
can lead to<br>
> a StackOverflowError in case there are many chained
subscriptions.<br>
> Running the following code demonstrates this:<br>
><br>
> ```<br>
> void testSubs() {<br>
> SimpleStringProperty prop = new
SimpleStringProperty("simpel");<br>
> Subscription s = prop.subscribe(() -> {});<br>
> for (int i = 0; i < 100000; i++) {<br>
> Subscription t = prop.subscribe(() -> {});<br>
> s = s.and(t);<br>
> }<br>
> s.unsubscribe();<br>
> }<br>
> ```<br>
><br>
> This results in<br>
> ```<br>
> java.lang.StackOverflowError<br>
> at<br>
>
<a class="moz-txt-link-abbreviated" href="mailto:javafx.base@26-ea/javafx.util.Subscription.lambda$and$0">javafx.base@26-ea/javafx.util.Subscription.lambda$and$0</a>(Subscription.java:103)<br>
> at<br>
>
<a class="moz-txt-link-abbreviated" href="mailto:javafx.base@26-ea/javafx.util.Subscription.lambda$and$0">javafx.base@26-ea/javafx.util.Subscription.lambda$and$0</a>(Subscription.java:103)<br>
> at<br>
>
<a class="moz-txt-link-abbreviated" href="mailto:javafx.base@26-ea/javafx.util.Subscription.lambda$and$0">javafx.base@26-ea/javafx.util.Subscription.lambda$and$0</a>(Subscription.java:103)<br>
> ...<br>
> ```<br>
><br>
> While it's unusual (and in most cases a very bad idea) to
chain that<br>
> many Subscriptions, I don't think this should give a
StackOverflow<br>
> Error. I believe it is better to avoid recursion in the<br>
> implementation. If people agree, I'll file an issue for
this.<br>
><br>
> - Johan<br>
</blockquote>
</div>
</blockquote>
</body>
</html>