SessionDestroyListeners are not called when an exception is thrown during detach

mperktold commented 3 weeks ago

Description of the bug

When a VaadinSession is destroyed, first all UIs are removed and then SessionDestroyListeners are called. When removing the UIs, all the onDetach hooks and DetachListeners will be notified about it.

If one of those hooks or listeners throws an exception, the SessionDestroyListeners will not be called.

Expected behavior

Exceptions in detach hooks and listeners should not prevent SessionDestroyListeners from being called.

Minimal reproducible example

public class SessionDestroyIssue extends VerticalLayout {

    public SessionDestroyIssue() {
        var vaadinSession = VaadinSession.getCurrent();
        vaadinSession.getService().addSessionDestroyListener(e -> System.out.println("Session destroyed"));
        var closeSession = new Button("close session", e -> vaadinSession.close());
        var badDiv = new Div();
        badDiv.addDetachListener(e -> {
            throw new IllegalStateException("test");
        });
        add(closeSession, badDiv);
    }
}

Versions

Vaadin / Flow version: 24.4.13
Java version: Temurin 21.0.5+11
OS version: Windows 11

mshabarov commented 3 weeks ago

Suppose Vaadin catches the exceptions when invoking detach listeners, how then it should handle this exception? Moreover, how should it tell session destroy listeners invoker to ignore this exception? Also, what happens if other listeners (else than detach listeners) throw an exception? I don't want to make this catch in framework and think it's better to handle/process an exception right away in the listener rather than rely on the framework.

mperktold commented 3 weeks ago

I agree that in general, we should just let the exception bubble up and not pretend that everything is fine. But I would rather apply that to the detach hooks, as they do not know whether they are invoked because the session is destroyed, or a component is removed from the dom, or maybe even just reattached.

Vaadin offers an API to get notified when the session is destroyed. When I use that API, I expect that to work reliably, without having to check every detach hook of my application, including third party tools. It should be the responsibility of the destroy event mechanism to guarantee that listeners are properly called, not the responsibility of every detach hook.

Now regarding what to do exactly and how to handle it, I think we should look at what VaadinService.fireSessionDestroy does now. It already catches exceptions thrown by the destroy listeners themselves to make sure all the listeners are called, even if one of them throws. The exception is handled by simply passing it to the session error handler:

https://github.com/vaadin/flow/blob/db675fe125e6949cf38a134a84f7f924dc1ac609/flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java#L703-L732

I think something similar would work also in the loop before where all the UIs are removed. For the sake of this issue, one big try catch around the whole loop would be enough, but in the same spirit I would say that failing to remove one UI should not prevent the next UI to be removed, so probably a try-catch for each UI would be best.

vaadin / flow