Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate hotswap support from the Hotswap Agent plugin #19261

Closed
Artur- opened this issue Apr 26, 2024 · 5 comments
Closed

Integrate hotswap support from the Hotswap Agent plugin #19261

Artur- opened this issue Apr 26, 2024 · 5 comments

Comments

@Artur-
Copy link
Member

Artur- commented Apr 26, 2024

Describe your motivation

The Hotswap agent plugin at https://github.com/HotswapProjects/HotswapAgent/tree/master/plugin/hotswap-agent-vaadin-plugin implements support for hot reloading of various parts of Flow but it has the issue that it tries to support all Flow versions at the same time, and that it is an external project so it is always lagging behind when Flow gets new features. As it tries to support many Flow versions, the code becomes quite complex.

Describe the solution you'd like

Implement the same approach already tested in Hilla here https://github.com/vaadin/hilla/blob/main/packages/java/endpoint/src/main/java/com/vaadin/hilla/Hotswapper.java#L34 and here https://github.com/vaadin/hilla/blob/main/packages/java/hilla-dev/src/main/java/com/vaadin/hilla/devmode/hotswapagent/HillaPlugin.java

In other words, make the Hotswap Agent plugin call a Flow method when hotswapping has taken place without doing any filtering or trying to figure out what needs to be done. Let the onHotswap method in Flow decide what needs to be done due to reloading of the given class(es).

This would have many benefits:

  1. The Hotswap Agent plugin does not need to support multiple Flow versions. It only needs to find onHotswap and different Flow versions can implement the method in different ways
  2. There is no need for strange reflection code to clear caches etc. The code in onHotswap will become much simpler than what is in the hotswap agent plugin
  3. The JRebel plugin can use the same method so that hotswapping will work equally well regardless of which product you use
@mshabarov
Copy link
Contributor

mshabarov commented May 8, 2024

Acceptance Criteria:

  • Flow has an API for updating routes, its implementation does the same as the existing logic does in hotswap-agent-vaadin-plugin - 3 SP
  • Platform (or Hilla, or Flow, should be checked which is better) has one single public method onHotswap that is called from hotswap-agent-vaadin-plugin
  • Vaadin's onHotswap method implementation delegates to corresponding hot swap methods in Flow and Hilla, depending on what is used in a project. - 5 SP (includes the previous point)
  • hotswap-agent-vaadin-plugin has still VaadinIntegration class, but the logic in there is called only if the onHotswap method is not found in Vaadin, otherwise the plugin does nothing, just calls the onHotswap method. This is needed for backwards compatibility between further versions of HA plugin and older Vaadin versions (before 24.5) - 5 SP
  • Hilla's HA plugin HillaPlugin is removed (basically remove the whole module hilla-dev) - 3 SP

Let's not pick these changes to older maintained Vaadin branches. If this feature will be needed for older Vaadin versions or requested by Customers, let's do it, otherwise ignore it for now and let VaadinIntegration class do the job.

@mcollovati
Copy link
Collaborator

Currently, Vaadin hotswap plugin reacts on @OnClassFileEvent to update the route registry and @OnClassLoadEvent(REDEFINE) to clear reflection cache and to trigger a browser page reload.
Hilla plugin reacts on OnClassLoadEvent(DEFINE and REDEFINE).

Vaadin plugin tries to batch file change events, instead of executing the logic immediately. I wonder if this could be applied to the other events as well.

So a common interface that should be implemented by Flow and Hilla (and in the future potentially also other parts of the platform) could be something like

public interface VaadinHotSwapper {

    default void onClassFileEvent(VaadinService vaadinService,
            Set<Class<?>> addedClasses, Set<Class<?>> modifiedClasses,
            Set<Class<?>> removedClasses) {
    }

    default void onClassFileEvent(VaadinSession session,
            Set<Class<?>> addedClasses, Set<Class<?>> modifiedClasses,
            Set<Class<?>> removedClasses) {
    }

    default void onClassLoadEvent(VaadinService vaadinService,
            boolean redefined, Set<Class<?>> classes) {
    }

    default void onClassLoadEvent(VaadinSession vaadinSession,
            boolean redefined, Set<Class<?>> classes) {
    }

    default void onResourceFileEvent(VaadinService vaadinService,
            Set<URI> addedResources, Set<URI> modifiedResources,
            Set<URI> removedResources) {
    }

    default void onResourceFileEvent(VaadinSession vaadinSession,
            Set<URI> addedResources, Set<URI> modifiedResources,
            Set<URI> removedResources) {
    }
}

I also added to the interface a method to react to @OnResourceFileEvent.
The three events should be called for both application (VaadinService) and active sessions (VaadinSession) in case changes to classes may impact session related components (e.g. SessionRouteRegistry).

On the Flow side, there will be a Hotswapper component as well, that will be the entry point called by the agent whose aim is to dispatch the events to the VaadinHotSwapper implementations (fetched with Lookup?). The Hotswapper should be initialized at VaadinService initialization (only in development mode) and will implement SessionInitListener and SessionDestroyListener to track VaadinSessions.

@Artur-
Copy link
Member Author

Artur- commented May 31, 2024

Why is it that OnClassFileEvent is used? Isn't that what often leads to race conditions as you do not know when the file has been compiled and is available on the classpath?

@Artur-
Copy link
Member Author

Artur- commented May 31, 2024

Vaadin plugin tries to batch file change events, instead of executing the logic immediately. I wonder if this could be applied to the other events as well.

When you edit the UI, I wonder if it isn't more important to get the changes in the browser as quickly as possible? I am not sure when you would edit multiple files at once.

In any case, the batching should be done in Vaadin then and not in the caller, so we can change it later on if needed

@mcollovati
Copy link
Collaborator

Why is it that OnClassFileEvent is used? Isn't that what often leads to race conditions as you do not know when the file has been compiled and is available on the classpath?

I don't know the reason. We should perhaps ask the guy who pushed this commit 😉

Anyway, I think you are right on the potential issues with the file events.
Also, the DELETE event doesn't seem to work properly; if I rename a class, I get a CREATE, MODIFY and DELETE, but if I just delete it an exception is raised in Hotswap agent and the event does not reach the plugin.

So it would probably make sense to only react on @OnClassLoadEvent and, for the current route registry updated logic consider DEFINE as an added class and REDEFINED as modified.

That said, we can most likely also drop the @OnResourceFileEvent, unless it can be helpful in cases like updating the translation property files.

About batching, probably the reason was that with file events you can get different events for the same file (e,g, CREATE, MODIFY and DELETE as said previously) for a single change, but the order seems not to be predictable.
So, as you propose, lets every single event to be propagated to the Vaadin component, and possibly handle a queue there

@mshabarov mshabarov moved this from 🟢Ready to Go to ⚒️ In progress in Vaadin Flow ongoing work (Vaadin 10+) Jun 5, 2024
mcollovati added a commit that referenced this issue Jun 11, 2024
mcollovati added a commit that referenced this issue Jun 18, 2024
Adds API to integrate with hotswap agents and to allow plugging class change
reload plugins.
The hotswapper also tries to refresh the views instead of reloading the
page, if PUSH feature is enabled.

Part of #19261
Part of #19262
mcollovati added a commit to mcollovati/HotswapAgent that referenced this issue Jun 21, 2024
Updates the Vaadin plugin to delegate reload operations to the
Vaadin Hotswapper component introduced in 24.5.
Backward compatibility with older Vaadin versions is preserved
by detecting the absence of the Hotswapper component and falling
back to the original behavior.

Part of vaadin/flow#19261
mshabarov added a commit that referenced this issue Jun 26, 2024
* feat: add a common API to intergrate with hotswap tools

Adds API to integrate with hotswap agents and to allow plugging class change
reload plugins.
The hotswapper also tries to refresh the views instead of reloading the
page, if PUSH feature is enabled.

Part of #19261
Part of #19262

* don't refresh is navigation has not yet happened

* apply review suggestions

* ignore events after VaadinService is destroyed

* Update flow-server/src/main/java/com/vaadin/flow/router/internal/RouteRegistryHotswapper.java

---------

Co-authored-by: Mikhail Shabarov <[email protected]>
@mshabarov mshabarov added this to Roadmap Jul 1, 2024
@github-project-automation github-project-automation bot moved this to Under consideration in Roadmap Jul 1, 2024
@mshabarov mshabarov moved this from Under consideration to September 2024 (24.5) in Roadmap Jul 1, 2024
skybber pushed a commit to HotswapProjects/HotswapAgent that referenced this issue Jul 1, 2024
Updates the Vaadin plugin to delegate reload operations to the
Vaadin Hotswapper component introduced in 24.5.
Backward compatibility with older Vaadin versions is preserved
by detecting the absence of the Hotswapper component and falling
back to the original behavior.

Part of vaadin/flow#19261
mcollovati added a commit to vaadin/hilla that referenced this issue Jul 1, 2024
Implements Flow hotswap API and removes the hotswap agent plugin.

Part of vaadin/flow#19261
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: September 2024 (24.5)
Development

No branches or pull requests

3 participants