StackOverflowError in recursive Subscription.unsubscribe

[John Hendrikx]

It's an interesting problem. I wrote some test cases, and came to this
solution to allow large amounts of chained subscriptions, while
maintaining Subscriptions as immutable constructs that will only
unsubscribe exactly the right subscriptions when using older references:

defaultSubscription and(Subscription other) {

Objects.requireNonNull(other, "other cannot be null");

returnnewNodeSubscription(this, other);

}

// TODOmove to package to make it package private

staticclassNodeSubscription implementsSubscription {

privatefinalSubscription s1;

privatefinalSubscription s2;

publicNodeSubscription(Subscription s1, Subscription s2) {

booleanshouldSwap = s1 instanceofNodeSubscription; // attempt to avoid
growing stack as much as possible

this.s1= shouldSwap ? s2 : s1;

this.s2= shouldSwap ? s1 : s2;

}

@Override

publicvoidunsubscribe() {

List<Subscription> stack = newArrayList<>();

stack.add(this);

while(!stack.isEmpty()) {

Subscription s = stack.removeLast();

if(s instanceofNodeSubscription n) {

stack.add(n.s1);

stack.add(n.s2);

}

else{

s.unsubscribe();

}

}

}

}

Note: as prescribed by the specification, `unsubscribe` is idempotent.

--John


On 13/08/2025 22:11, John Hendrikx 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 at 26-ea/javafx.util.Subscription.lambda$and$0(Subscription.java:103)
>> at
>> javafx.base at 26-ea/javafx.util.Subscription.lambda$and$0(Subscription.java:103)
>> at
>> javafx.base at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20250813/d284d6ea/attachment-0001.htm>