Support JSR-356 websockets with Spring Boot

[Artur-]

Depends on #3740

---

This change is 

[caalador]

Reviewed 4 of 11 files at r1.
Review status: 4 of 11 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

---

flow-spring-addon/src/main/java/com/vaadin/flow/spring/VaadinWebsocketEndpointExporter.java, line 1 at r1 (raw file):

package com.vaadin.flow.spring;

Missing Copyright header.

---

Comments from Reviewable

[Artur-]

Review status: 3 of 6 files reviewed at latest revision, 1 unresolved discussion.

---

flow-spring-addon/src/main/java/com/vaadin/flow/spring/VaadinWebsocketEndpointExporter.java, line 1 at r1 (raw file):

Previously, caalador wrote…

Missing Copyright header.

Done.

---

Comments from Reviewable

[denis-anisimov]

Reviewed 3 of 11 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

flow-spring-addon/src/main/java/com/vaadin/flow/spring/VaadinWebsocketEndpointExporter.java, line 42 at r2 (raw file):

At least Jetty uses a

Quoted 5 lines of code…

> // ServletContainerInitializer to initialize its websocket support > // and that one might or might not have been run before this code. > // Need to bail out and let JSR356WebsocketInitializer handle it > // through its listener when the websocket support is definitely > // available.

Does it mean that there might be several attempts to initialize websocket endpoint (this code and probably web container)?

Is it possible to postpone somehow our initialization ( this code) when we may definitely check that there is already websocket endpoint ?

E.g. please see VaadinServletContextInitializer class which adds ServletContextListener listeners instead of executing the logic which scans Vaadin types immediately (kind of similar situation: we need this logic only for Spring Boot application, in a web container it's executed automatically).

It prevents our code execution if everything is already done automatically by the web server container.
I'm not sure whether it's possible or not here. Just asking.

---

flow-tests/test-spring-common/src/main/java/com/vaadin/flow/spring/test/PushView.java, line 32 at r2 (raw file):

e -> {

Quoted 11 lines of code…

> add(new Paragraph("Hello")); > new Thread(() -> { > try { > Thread.sleep(100); > } catch (InterruptedException e1) { > } > ui.access(() -> { > add(new Paragraph("World")); > }); > }).start(); > }

Could you please extract this into a method ?
Too many nested blocks and very big lambda. Hard to read.

---

flow-tests/test-spring-common/src/test/java/com/vaadin/flow/spring/test/PushIT.java, line 17 at r2 (raw file):

Thread.sleep(1000);

Fragile code.
May it be changed to waitUntil with increased timeout ?

How can we be sure that its websockets and not long-polling e.g.?
Or it doesn't matter ?

---

Comments from Reviewable

[Artur-]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

flow-spring-addon/src/main/java/com/vaadin/flow/spring/VaadinWebsocketEndpointExporter.java, line 42 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

At least Jetty uses a
// ServletContainerInitializer to initialize its websocket support
// and that one might or might not have been run before this code.
// Need to bail out and let JSR356WebsocketInitializer handle it
// through its listener when the websocket support is definitely
// available.

Does it mean that there might be several attempts to initialize websocket endpoint (this code and probably web container)?

Is it possible to postpone somehow our initialization ( this code) when we may definitely check that there is already websocket endpoint ?

E.g. please see VaadinServletContextInitializer class which adds ServletContextListener listeners instead of executing the logic which scans Vaadin types immediately (kind of similar situation: we need this logic only for Spring Boot application, in a web container it's executed automatically).

It prevents our code execution if everything is already done automatically by the web server container.
I'm not sure whether it's possible or not here. Just asking.

As far as I know, this should be THE place to do it with Spring Boot and I am planning to create a Spring Boot issue about serverContainer being null here if I can reproduce it outside Flow.

A ServletContextListener does not work because of ServletContextListeners are added from another ServletContextListener when running in Spring Boot. The added ServletContextListener then does not have access to e.g. Servlet registrations as stated by the servlet spec.

---

flow-tests/test-spring-common/src/main/java/com/vaadin/flow/spring/test/PushView.java, line 32 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

e -> {
add(new Paragraph("Hello"));
new Thread(() -> {
try {
Thread.sleep(100);
} catch (InterruptedException e1) {
}
ui.access(() -> {
add(new Paragraph("World"));
});
}).start();
}

Could you please extract this into a method ?
Too many nested blocks and very big lambda. Hard to read.

Done.

---

flow-tests/test-spring-common/src/test/java/com/vaadin/flow/spring/test/PushIT.java, line 17 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

Thread.sleep(1000);

Fragile code.
May it be changed to waitUntil with increased timeout ?

How can we be sure that its websockets and not long-polling e.g.?
Or it doesn't matter ?

Fixed and added comment about how we know it's websockets

---

Comments from Reviewable

[denis-anisimov]

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

flow-spring-addon/src/main/java/com/vaadin/flow/spring/VaadinWebsocketEndpointExporter.java, line 42 at r2 (raw file):

Previously, Artur- (Artur) wrote…

As far as I know, this should be THE place to do it with Spring Boot and I am planning to create a Spring Boot issue about serverContainer being null here if I can reproduce it outside Flow.

A ServletContextListener does not work because of ServletContextListeners are added from another ServletContextListener when running in Spring Boot. The added ServletContextListener then does not have access to e.g. Servlet registrations as stated by the servlet spec.

Sure.
I didn't mean that ServletContextListener should be used here anyhow.
I just meant the approach: IF it's possible to find a place (somehow using some API) where it's definitely known that websocket endpoint already exists or will never exist.

If it's not possible then OK.

---

Comments from Reviewable

[denis-anisimov]

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[Artur-]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

flow-spring-addon/src/main/java/com/vaadin/flow/spring/VaadinWebsocketEndpointExporter.java, line 42 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

Sure.
I didn't mean that ServletContextListener should be used here anyhow.
I just meant the approach: IF it's possible to find a place (somehow using some API) where it's definitely known that websocket endpoint already exists or will never exist.

If it's not possible then OK.

But the logic here now makes no sense as the if says to bail out if serverContainer is defined...

---

Comments from Reviewable

[denis-anisimov]

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

flow-spring-addon/src/main/java/com/vaadin/flow/spring/VaadinWebsocketEndpointExporter.java, line 42 at r2 (raw file):

Previously, Artur- (Artur) wrote…

But the logic here now makes no sense as the if says to bail out if serverContainer is defined...

Does serverContainer presence mean that websocket support is already available ?
Or I'm not sure what you means......

The comment is not blocking anyway.

---

Comments from Reviewable

[Artur-]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

flow-spring-addon/src/main/java/com/vaadin/flow/spring/VaadinWebsocketEndpointExporter.java, line 42 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

Does serverContainer presence mean that websocket support is already available ?
Or I'm not sure what you means......

The comment is not blocking anyway.

Yes, the spec says that the server adds its own ServerContainer implementation as a servlet context attribute: javax.websocket.server.ServerContainer. This is what getServerContainer() returns and it must be != null to be able to register websocket endpoints

---

Comments from Reviewable

[denis-anisimov]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

flow-spring-addon/src/main/java/com/vaadin/flow/spring/VaadinWebsocketEndpointExporter.java, line 42 at r2 (raw file):

Previously, Artur- (Artur) wrote…

Yes, the spec says that the server adds its own ServerContainer implementation as a servlet context attribute: javax.websocket.server.ServerContainer. This is what getServerContainer() returns and it must be != null to be able to register websocket endpoints

Alright 👍

---

Comments from Reviewable

[denis-anisimov]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

flow-spring-addon/src/main/java/com/vaadin/flow/spring/VaadinWebsocketEndpointExporter.java, line 1 at r1 (raw file):

Previously, Artur- (Artur) wrote…

Done.

@caalador ? Please resolve the comment

---

Comments from Reviewable

[caalador]

---

Reviewed 2 of 4 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[denis-anisimov]

---

Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable