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

@Push broken in OSGi for Flow V12.0.6 #5081

Closed
enver-haase opened this issue Feb 17, 2019 · 5 comments
Closed

@Push broken in OSGi for Flow V12.0.6 #5081

enver-haase opened this issue Feb 17, 2019 · 5 comments
Assignees
Labels
BFP Bugfix priority, also known as Warranty bug OSGi
Milestone

Comments

@enver-haase
Copy link
Contributor

  • Description of the bug

screenshot 2019-02-17 at 21 01 36

  • Minimal reproducible example
    Use base-starter-flow-osgi and add @Push annotation to MainView.java
  • Expected behavior
    I added a small ServiceListener and I would expect any service changes I could immediately push into the browser (of course, using ui.access()).
  • Actual behavior
    See screenshot.
  • Versions:
    • Vaadin / Flow version
      12.0.6
    • Java version
      1.8.0_181
    • OS version
      OSX 10.14.3
    • Browser version (if applicable)
      Chrome Version 72.0.3626.109 (Official Build) (64-bit)
@pleku pleku added bug OSGi BFP Bugfix priority, also known as Warranty labels Feb 18, 2019
@denis-anisimov denis-anisimov self-assigned this Feb 21, 2019
@denis-anisimov
Copy link
Contributor

denis-anisimov commented Feb 25, 2019

The error message is a result of unavailable push resources (vaadinPush.js) via HTTP.
There are static web resource files in flow-push module which should be accessible via HTTP.
This issue is fixed by #5112.

But this is far away from the end of the story.

Push doesn't work if only the push resources are registered.

I don't know the exact reason why but here are some notes (basically for myself but may be useful for understanding of issues overall in the future).

  • Atmosphere haws some ServletContainerInitializers. They are exposed as services available via ServiceLoader. The problem is that OSGi doesn't have to use those ServletContainerInitializer at all: at least Felix doesn't execute them. The situation is quite similar to Route registration (same issue with ServletContainerInitializers).
  • It seems that org.atmosphere.cpr.ContainerInitializer has to be executed to initialize AtmosphereFramework. And I believe it's not executed. At least I don't see a log message Initializing AtmosphereFramework inside OSGi.
  • Another thing is our JSR356WebsocketInitializer class which is ServletContextListener. It's the same issue. It's not registered alone but it's aggregated by the ServletContextListeners class which is ServletContextListener annotated with @WebListener. And this listener is not executed as well in OSGi.

I believe those two classes are required to make Push work.
So we need to solve those issues.

I'm trying to use HTTP Whiteboard specification to register ServletContextListener:

Dictionary<String, Object> props = new Hashtable<String, Object>();
        props.put("osgi.http.whiteboard.listener", true);

        context.registerService(ServletContextListener.class,
                new ServletContextListener() {

                    @Override
                    public void contextInitialized(ServletContextEvent sce) {
                        System.out.println("Init");
                    }

                    @Override
                    public void contextDestroyed(ServletContextEvent sce) {
                    }
                }, props);

And this doesn't work.
I see a message in the output:

[DEBUG] [ServiceReference 4 from bundle 21 : com.vaadin.flow.server:1.5.0.201902251245 ref=[javax.servlet.ServletContextListener] properties={objectClass=[javax.servlet.ServletContextListener], osgi.http.whiteboard.listener=true, service.bundleid=21, service.id=4, service.scope=singleton}] Ignoring invalid Listener service

I've been trying register a servlet using HTT Whiteboard specification and without it. The result is the same in both cases. The error somehow relates to HTTP Whiteboard impl (in Felix) because if I remove
props.put("osgi.http.whiteboard.listener", true); then there is no the error message.

Now, here are several implementation notes:

  • ContainerInitializer impl uses code like this : c.addListener(new ServletRequestListener() { where c is instance of ServletContext. It doesn't work in OSGi at all because https://osgi.org/specification/osgi.cmpn/7.0.0/service.http.whiteboard.html#d0e119708 : addListener methods are not supported by the ServletContext impl in OSGi and I can confirm it : inside Felix addListener throws an exception regardless of the way of servlet registration. It means that atmosphere ContainerInitializer cannot be used as is in OSGi. We will need something to do with it. May be proxy ServletContext and delegate addListener calls to registerService .
  • We need a way to get a ServletContext instance at initialization phase. So figure out how ServletContextListener can be registered or find any other way to access ServletContext instance to be able to execute the code in ContainerInitializer and JSR356WebsocketInitializer.
  • Our VaadinBundleTracker finds atmosphere ContainerInitializer class during scanning flow-server bundle for some reason. So technically we may find ContainerInitializer class during class scanning and load it. Not sure whether we may instantiate it and execute it's code. May be we should use ServiceLoader instead.

Anyway there are a lot of issues with Push. It won't be easy to solve all of them.

I wonder whether Push works in Framework 8 because it should have similar issues.
@Artur- , do you know anything about Push in FW8 (any known issues, whether it has been ever tested) ?

@denis-anisimov
Copy link
Contributor

denis-anisimov commented Feb 26, 2019

Updates:

As a result:

  • Push doesn't work but not because non functional org.atmosphere.cpr.ContainerInitializer.
  • JSR356WebsocketInitializer still needs to be involved somehow but Push doesn't work even with JSR356WebsocketInitializer doing its work.

@denis-anisimov
Copy link
Contributor

We still need to figure out what to do with ServletContextListener but for now I would like to get Push working in some (may be very hacky and ugly) way.

So to be able to see the current state one should :

private static class FixedViewServlet extends ViewTestServlet {
        @Override
        public void init(ServletConfig servletConfig) throws ServletException {
            super.init(servletConfig);

            getService().setClassLoader(getClass().getClassLoader());

            JSR356WebsocketInitializer jsr356Initializer = new JSR356WebsocketInitializer();
            jsr356Initializer.contextInitialized(
                    new ServletContextEvent(servletConfig.getServletContext()));
        }
    }
  • Rebuild Flow
  • Go to flow/flow-tests/servlet-containers/felix-jetty/ and run mvn verify -DskipTests.
  • The previous command line prepares the Felix distribution to run the tests.
  • Go to target/felix-framework-6.0.0 : cd target/felix-framework-6.0.0.
  • Run Felix Go : java -jar bin/felix.jar.
  • Open browser http://localhost:8888/view/com.vaadin.flow.uitest.ui.UpdateDivView.

The result is: page is reloading all the time.
The same view works as it should in plain Jetty: the counter is updated from 0 to 50 with a short delay between number changes.

Here in this hack I've used JSR356WebsocketInitializer instance being applied to a servlet context directly in servlet's init method and since the initializer doesn't use any ServletContext features that may be used only during context initialization it should work in the same way as being called as * javax.servlet.ServletContextListener.
At least that's my assumption.
But Push still doesn't work.
So either my assumption is wrong and JSR356WebsocketInitializer doesn't do something or there is some functionality which is not called for some reason in OSGi.

@Artur- , could you please take a look at it. At least may be you can say the reason why Push doesn't work with this approach.

@denis-anisimov
Copy link
Contributor

The base-starter-flow-osgi project doesn't have websocket-api dependency.
This dependency is required to enable websocket:

<dependency>
            <groupId>javax.websocket</groupId>
            <artifactId>javax.websocket-api</artifactId>
            <version>1.1</version>
            <scope>provided</scope>
        </dependency>

That still doesn't enable Push for the project (push via WS).
An exception is thrown when the Activator is modified to configure Atmosphere with JSR356WebsocketInitializer:

java.lang.IllegalStateException: Unable to configure jsr356 at that stage. ServerContainer is null
	at org.atmosphere.container.JSR356AsyncSupport.<init>(JSR356AsyncSupport.java:53)
	at org.atmosphere.container.JSR356AsyncSupport.<init>(JSR356AsyncSupport.java:42)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
	at org.atmosphere.cpr.DefaultAsyncSupportResolver.newCometSupport(DefaultAsyncSupportResolver.java:237)
	at org.atmosphere.cpr.DefaultAsyncSupportResolver.resolveWebSocket(DefaultAsyncSupportResolver.java:308)
	at org.atmosphere.cpr.DefaultAsyncSupportResolver.resolve(DefaultAsyncSupportResolver.java:294)
	at org.atmosphere.cpr.AtmosphereFramework.autoDetectContainer(AtmosphereFramework.java:2092)
	at org.atmosphere.cpr.AtmosphereFramework.init(AtmosphereFramework.java:914)
	at org.atmosphere.cpr.AtmosphereFramework.init(AtmosphereFramework.java:838)
	at com.vaadin.flow.server.communication.PushRequestHandler.initAtmosphere(PushRequestHandler.java:218)

The exception is caused by the fact that ServletContext instance has no "javax.websocket.server.ServerContainer" attribute.

Apparently this attribute should be set by some initializer. It's not clear which initializer should set it.
There are two dependencies :

<dependency>
            <groupId>org.eclipse.jetty.websocket</groupId>
            <artifactId>javax-websocket-server-impl</artifactId>
            <version>9.4.12.v20180830</version>
            <scope>provided</scope>
        </dependency>

        <dependency>
            <groupId>org.eclipse.jetty.websocket</groupId>
            <artifactId>javax-websocket-client-impl</artifactId>
            <version>9.4.12.v20180830</version>
            <scope>provided</scope>
        </dependency>

which should theoretically add jetty websocket implementation bundle.
And javax-websocket-server-impl contains WebSocketServerContainerInitializer which sets "javax.websocket.server.ServerContainer" attribute.
But this class is ServletContainerInitializer (and once again there is an issue to execute in in OSGi) and it uses ContextHandler class whose instance is not available inside Felix Jetty.

So it's still not clear which class inside Felix Jetty should initialize javax.websocket.server.ServerContainer.

@denis-anisimov
Copy link
Contributor

This ticket in its original description should be fixed by #5112.

In fact Push still doesn't work out of the box because default transport is Websocket.
Websocket requires a very non trivial additional configuration in the project which I have no even idea about.

But if transport is set to Long polling: @Push(transport = Transport.LONG_POLLING) then the fix is provided.

The issue for websockets is a completely different thing and I will create a separate ticket for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFP Bugfix priority, also known as Warranty bug OSGi
Projects
None yet
Development

No branches or pull requests

3 participants