DetachListeners and onDetach hooks are not called when an exception is thrown during detach of contained components

[mperktold]

Description of the bug

This is related to #20293, but focuses on the DetachEvent rather then the SessionDestroyEvent.

When a component is removed from the view, the whole component subtree will be notified bottom-up about this DetachEvent. If one of the detach hooks or listeners of "lower" components throws an exception, the components higher up the tree will never get notified.

This can lead to resource leaks, because onDetach is a good place to release resources (which are typically aquired in onAttach).

Currently, the only way to make sure a custom onDetach hook is always called is to check every implementation of onDetach of components further down the tree and make sure they don't throw, possibly using try-catch. And there might be third-party components which you don't control directly.

Expected behavior

Ideally, there should be try-finally-like semantics for DetachEvent, meaning that the corresponding hooks and listeners should always be called, even if an exception is thrown from detach hooks further down the tree.

As I said in #20293, it shouldn't be the responsibility of these other components to make sure another component higher up the tree can clean up its resources. Instead, this should be the responsibility of that component itself, and Vaadin should provide the mechanisms to make that possible.

I am not sure what exactly should happen when an exception is thrown, though. I guess a good start is to wrap every invocation of a detach hook or listener in a try-catch block. In the catch block, one option would be to simply pass all exceptions to VaadinSession.getErrorHandler(). Another option would be to remember the first exception and to add all following exceptions as suppressed to it, so the first exception can be rethrown once the whole detach process has completed.

Minimal reproducible example

public class DetachTest extends VerticalLayout {

    public DetachTest() {
        var badDiv = new Div();
        badDiv.addDetachListener(e -> {
            throw new IllegalStateException("Test");
        });
        add(badDiv);
    }

    @Override
    protected void onDetach(DetachEvent detachEvent) {
        super.onDetach(detachEvent);
        System.out.println("super important cleanup logic");
        Notification.show("detached");
    }
}

1. Add this view to your project, with at least one other view
2. Open this view in your browser
3. Navigate to another view

onDetach will not print anything to the console, nor will it show the notification.

Versions

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

[Legioth]

Agreed. These types of listeners should be resilient so that application code can rely on those being called when appropriate even if other parts of the application misbehaves.

But this mainly applies for the types of listeners that are typically used for cleanup, e.g. component detach, session destroy and service destroy. There's no need to put the same effort into making e.g. click listeners resilient.

The best solution would probably be to collect all exceptions from a the whole recursive detach operation and throw them once the detach is completed. If multiple exceptions are caught, then we should throw the first one but attach all the others using addSuppressed.

[mperktold]

But this mainly applies for the types of listeners that are typically used for cleanup, e.g. component detach, session destroy and service destroy. There's no need to put the same effort into making e.g. click listeners resilient.

Makes sense. I can confirm that this would be enough for our purposes.

The best solution would probably be to collect all exceptions from a the whole recursive detach operation and throw them once the detach is completed. If multiple exceptions are caught, then we should throw the first one but attach all the others using addSuppressed.

Right, this is exactly what I intended with the second option.

[mshabarov]

Makes sense to do #20577 (comment) and fix the linked issue #20293 in one go, but let us first investigate what the implementation could be. The concern is that it can require a new method or behaviour change.

Overall, limiting the scope to only detach/destroy listeners and re-throw the original exception sounds as a good compromise.

[tepi]

Looks like for SessionDestroyListener, this is already implemented: https://github.com/vaadin/flow/blob/main/flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java#L720-L732

[mperktold]

Great that this has been fixed, thanks! 👍

Just out of curiosity: Why did you choose to pass exceptions to the ErrorHandler instead of collecting and rethrowing them? Is it because the ServiceDestroyListener already did something similar, as pointed out by @tepi?