On Mon, 10 Mar 2025 09:27:29 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> John Hendrikx has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Small fixes from review comments
>> - Use switch expression where reasonable
>> - Update docs regarding NullPointerExceptions
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerList.java
> line 174:
>
>> 172: */
>> 173:
>> 174: throw new StackOverflowError("non-converging value
>> detected in value modifying listeners on " + observableValue + "; original
>> value was: " + oldValue);
>
> We shouldn't throw errors, even if that's what the old implementation would
> implicitly do (not that it matters, no sane application would ever try to
> catch a `StackOverflowError` as part of its normal control flow). I think a
> `RuntimeException` is in order.
>
> Also maybe make the message a bit clearer:
> `<observableValue> was changed during the invocation of multiple listeners,
> but the new values diverge [original value: <oldValue>]`
Maybe it should only be a warning log. Any exception a listener may throw is
currently only logged so other listeners may still continue to work correctly.
Throwing an `Error` was a nice way around that, but does potentially leave some
listeners uninformed of a change or invalidation (same as `ExpressionHelper` in
that regard).
If we go for a runtime exception, I'd need to allow the exception to pass
through, but the same thing still applies that breaking it off with an
exception may leave some listeners uninformed. So I think we need to decide,
how "bad" is this problem, and does it warrant terminating the application.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1987032716
