Fix NPE related to use of Attributes.Wrapper getAttributeNameSet()

[mpetzold]

Jetty version

9.4.30

Java version

openjdk version "11.0.6" 2020-01-14
OpenJDK Runtime Environment (build 11.0.6+10-post-Debian-1deb10u1)
OpenJDK 64-Bit Server VM (build 11.0.6+10-post-Debian-1deb10u1, mixed mode, sharing)

OS type/version

Linux

Description

I am using Vaadin 14.2.0. I switched from Jetty 9.4.26 to 9.4.30. WebSocket with Vaadin / Atmosphere seems to make some trouble. The NPE seems to be related to Jetty, because I only changed the Jetty libraries. This error did not occur before:

2020-06-19 20:46:48.001 WARN org.eclipse.jetty.server.HttpChannel - /
java.lang.NullPointerException: null
at java.base/java.util.concurrent.ConcurrentHashMap.putVal(ConcurrentHashMap.java:1011)
at java.base/java.util.concurrent.ConcurrentHashMap.put(ConcurrentHashMap.java:1006)
at org.atmosphere.cpr.AtmosphereRequest$LocalAttributes.put(AtmosphereRequest.java:589)
at org.atmosphere.cpr.AtmosphereRequestImpl.wrap(AtmosphereRequestImpl.java:1421)
at com.vaadin.flow.server.communication.PushRequestHandler.handleRequest(PushRequestHandler.java:251)
at com.vaadin.flow.server.communication.PushRequestHandler.handleSessionExpired(PushRequestHandler.java:279)
at com.vaadin.flow.server.VaadinService.handleSessionExpired(VaadinService.java:1668)
at com.vaadin.flow.server.VaadinService.handleRequest(VaadinService.java:1555)
at com.vaadin.flow.server.VaadinServlet.service(VaadinServlet.java:247)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:790)
at Proxy1d934a1e_9b31_48c3_91cc_196ba0f3765f.service(Unknown Source)
at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:763)
at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1631)
at org.eclipse.jetty.websocket.server.WebSocketUpgradeFilter.doFilter(WebSocketUpgradeFilter.java:226)
at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1618)
at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:549)
at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:233)
at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:1610)
at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:233)
at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1369)
at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:188)
at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:489)
at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:1580)
at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:186)
at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1284)
at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141)
at org.eclipse.jetty.server.handler.ContextHandlerCollection.handle(ContextHandlerCollection.java:234)
at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127)
at org.eclipse.jetty.server.Server.handle(Server.java:501)
at org.eclipse.jetty.server.HttpChannel.lambda$handle$1(HttpChannel.java:383)
at org.eclipse.jetty.server.HttpChannel.dispatch(HttpChannel.java:556)
at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:375)
at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:272)
at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:311)
at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:103)
at org.eclipse.jetty.io.ssl.SslConnection$DecryptedEndPoint.onFillable(SslConnection.java:543)
at org.eclipse.jetty.io.ssl.SslConnection.onFillable(SslConnection.java:398)
at org.eclipse.jetty.io.ssl.SslConnection$2.succeeded(SslConnection.java:161)
at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:103)
at org.eclipse.jetty.io.ChannelEndPoint$1.run(ChannelEndPoint.java:104)
at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:336)
at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:313)
at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:171)
at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.run(EatWhatYouKill.java:129)
at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:375)
at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:806)
at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:938)
at java.base/java.lang.Thread.run(Thread.java:834)

[joakime]

The sequence here ...

2020-06-19 20:46:48.001 WARN org.eclipse.jetty.server.HttpChannel - /
java.lang.NullPointerException: null
at java.base/java.util.concurrent.ConcurrentHashMap.putVal(ConcurrentHashMap.java:1011)
at java.base/java.util.concurrent.ConcurrentHashMap.put(ConcurrentHashMap.java:1006)
at org.atmosphere.cpr.AtmosphereRequest$LocalAttributes.put(AtmosphereRequest.java:589)
at org.atmosphere.cpr.AtmosphereRequestImpl.wrap(AtmosphereRequestImpl.java:1421)
at com.vaadin.flow.server.communication.PushRequestHandler.handleRequest(PushRequestHandler.java:251)
at com.vaadin.flow.server.communication.PushRequestHandler.handleSessionExpired(PushRequestHandler.java:279)
at com.vaadin.flow.server.VaadinService.handleSessionExpired(VaadinService.java:1668)
at com.vaadin.flow.server.VaadinService.handleRequest(VaadinService.java:1555)
at com.vaadin.flow.server.VaadinServlet.service(VaadinServlet.java:247)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:790)
at Proxy1d934a1e_9b31_48c3_91cc_196ba0f3765f.service(Unknown Source)
at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:763)
at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1631)
at org.eclipse.jetty.websocket.server.WebSocketUpgradeFilter.doFilter(WebSocketUpgradeFilter.java:226)

Tells us that this wasn't a websocket upgrade, as it passed the WebSocketUpgradeFilter and hit the Filter chain's doFilter() request.

The rest of the stacktrace seems to show something bad in Atmosphere's handling of wrapped request attributes.
What version of Atmosphere are you using?

[mpetzold]

Vaadin 14.2.1 uses atmosphere-runtime-2.4.30.slf4jvaadin1.jar

[mpetzold]

It is patched to work in Vaadin and I cannot change only this to another version.

[mpetzold]

I just updated to Vaadin 14.2.1 and still have this error.

[joakime]

The NPE is produced from Atmosphere's LocalAttributes code.
It's attempting to set a LocalAttribute where either the key is null, or the value is null.
That's what's causing the NPE on ConcurrentHashMap.put()

The code that handles AtmosphereRequestImpl.wrap() in Atrmosphere 2.4.30 is ...

https://github.com/Atmosphere/atmosphere/blob/atmosphere-project-2.4.30/modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereRequestImpl.java#L1410-L1424

Would recommend that you file an issue at https://github.com/Atmosphere/atmosphere/issues as well.
And please link back to this issue, as we are curious as to what is causing this.

[joakime]

I'm looking at the recent changes for memory usage on Attributes from Issue #4861

@gregw @lachlan-roberts does anything on this issue jump out at you?

[joakime]

@mpetzold if you can reproduce easily, can you put a breakpoint on the AtmosphereRequestImpl.wrap() and see if any of the attribute names or values you see from the request are null?

        Builder b = new Builder();
        Enumeration<String> e = request.getAttributeNames();
        String s;
        while (e.hasMoreElements()) {
            s = e.nextElement();  // <-- THIS LINE
            b.localAttributes.put(s, attributeWithoutException(request, s)); // <-- and the return from attributeWithoutException()
        }
        return b.request(request).build();

[joakime]

I tested behaviors of 9.4.26 vs 9.4.30 in regards to Request Attributes.
They behave the same for the following two testcases.

protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
{
    // Should throw an immediate NPE
    request.setAttribute(null, "Foo");
}

protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
{
    request.setAttribute("Foo", "bogus");
    // Should delete the "Foo" attribute
    request.setAttribute("Foo", null);

    response.setContentType("text/plain");
    response.setCharacterEncoding("utf-8");
    // Should result in a response content of "null"
    response.getOutputStream().println("" + request.getAttribute("Foo"));
}

So that tells me that the Servlet API interface to the AttributesMap cannot include a null name, and properly handles null value.
What other means could result in a null on the AttributeNames is still a mystery.

[joakime]

@mpetzold do you have a project that we can use to replicate this issue on our side?

I took a stab at attempting to build vaadin-platform, but it REALLY doesn't like Ubuntu 18.04 or this 64 core machine i'm on.
Plus it forces selenium which has never worked on my bleeding edge chromium builds (chromedriver doesn't exist for these builds).

[mpetzold]

Updated to Atmosphere 2.6.0 with no change.

[mpetzold]

Errors on client side (related to Vaadin and also posted at Vaadin issue!):

vaadinPush-min.js?v=2.2.1:1 WebSocket connection to 'wss:///?v-r=push&v-uiId=1&v-pushId=62bf44f6-45f0-468b-ad0e-c48a333e5f76&X-Atmosphere-tracking-id=0&X-Atmosphere-Framework=2.3.2.vaadin1-javascript&X-Atmosphere-Transport=websocket&X-Atmosphere-TrackMessageSize=true&Content-Type=application/json;%20charset=UTF-8&X-atmo-protocol=true' failed: Error during WebSocket handshake: Unexpected response code: 500
aw @ vaadinPush-min.js?v=2.2.1:1

Websocket closed, reason: Connection was closed abnormally (that is, with no close frame being sent). - wasClean: false
log @ vaadinPush-min.js?v=2.2.1:1

[mpetzold]

I'm still convinced this bug is related to a change in Jetty implementation between 9.4.26 and 9.4.30. It is hard to build a simple test case.

ConcurrentHashMap: "Like Hashtable but unlike HashMap, this class does not allow null to be used as a key or value.". Was the underlying data structure changed in #4861?

[mpetzold]

btw. originally I updated Jetty because I received two PONGS after sending one PING (this websocket connection is not related to Vaadin). Now I only receive one PONG, so I am happy here. Side effect was then this issue with Vaadin / Atmosphere websocket connection. So, I have multiple websocket connections with different clients.

[mpetzold]

@gregw @lachlan-roberts Could there be a problem with your work in #4861?

It will take a lot of time to do a good test case for this. Maybe you could check this issue first?

Thanks!

[lachlan-roberts]

This does look like it could be related to the changes from #4867 or #4816. Seems likely that like one of the Attributes.Wrapper gives an attribute name in getAttributeNameSet() which then gives a null value when looked up with getAttribute().

This could be due to some issue in one of the Attributes.Wraper implementations, possibly one of the following:

1. We have overridden an attribute with null, and are still adding the attribute name when we do getAttributeNameSet().
2. If the attribute value is determined lazily when getAttribute() is called, we don't know whether the value is null or not until we call getAttribute() for it, so the name is added anyway.
3. If use the Attributes.Wrapper to override an attribute with a null value, the wrapped attributes may still contain the name in its attributeNameSet so it is still returned when the wrappers getAttributeNameSet() is called.

So we need to review each Attributes.Wraper implementation to determine if a problem exists. It would still be very helpful to know what attribute it is failing on to know exactly where the problem is.

[mpetzold]

@lachlan-roberts Is it ready? Do you have a patched jar, so I can check?

[lachlan-roberts]

@mpetzold Yes, if I'm correct these changes should fix your NPE, so it would be great if you could test it and verify whether it actually fixes the issue.

You can checkout the jetty-9.4.x-4985-AttributeNameSet branch locally and run, mvn clean install -DskipTests. Then you can find the jars in jetty-home with all the jars located at jetty-home/target/jetty-home/lib. I think all the changes should be in the jetty-server jar.

[mpetzold]

The NPE is gone. However, I still have a problem with the WebSocket connection of Vaadin / Atmosphere. This may be something else or related configuration.

[sbordet]

Fixed by #5004.