org.eclipse.jetty.server.Request throws NullPointerException if SessionHandler newHttpSession returns null

[gonphsn]

Jetty version
Jetty 9.4.32-snapshot as of September 28, 2020

Java version
D:\niagara\r410>java -version
java version "1.8.0_261"
Java(TM) SE Runtime Environment (build 1.8.0_261-b33)
Java HotSpot(TM) 64-Bit Server VM (build 25.261-b33, mixed mode)

OS type/version
Windows 10, amd64

Description
I'm not entirely sure if this is intentional behavior, but it seemed wrong, or at least overly painful, so please excuse if this was better suited as a question then a bug. I know the title seems like a 'well, duh' scenario, but this all takes place internally to the Request class where I can't change the behavior without overriding, so bear with me for a moment.

If the SessionHandler newHttpSession() functions returns null, as would be the case if an exception occurred:

    public HttpSession newHttpSession(HttpServletRequest request)
    {
        try
        {
           ...
        }
        catch (Exception e)
        {
            LOG.warn(e);
            return null;
        }
    }

or could be the case if the class was extended and the session creation failed for some application/subclass specific reason, then the cookie replacement handling that takes place in org.eclipse.jetty.server.Request:

    /*
     * @see javax.servlet.http.HttpServletRequest#getSession(boolean)
     */
    @Override
    public HttpSession getSession(boolean create)
    {

...

        _session = _sessionHandler.newHttpSession(this);
        HttpCookie cookie = _sessionHandler.getSessionCookie(_session, getContextPath(), isSecure());
        if (cookie != null)
            _channel.getResponse().replaceCookie(cookie);

will throw a NullPointerException in the getSessionCookie() function when the session extendedId is determined:

    public HttpCookie getSessionCookie(HttpSession session, String contextPath, boolean requestIsSecure)
    {
        if (isUsingCookies())
        {
            String sessionPath = (_cookieConfig.getPath() == null) ? contextPath : _cookieConfig.getPath();
            sessionPath = (StringUtil.isEmpty(sessionPath)) ? "/" : sessionPath;
            String id = getExtendedId(session);

...

    public String getExtendedId(HttpSession session)
    {
        Session s = ((SessionIf)session).getSession();
        return s.getExtendedId();
    }

Considering the amount of null condition protection elsewhere in the Request class, including null checks on the return value of getSession() itself, is it reasonable to think that this NullPointerException could be prevented with some basic assertion in session cookie calculation?

Consider the possibility that an IllegalStateException is thrown when creating the new session in AbstractSessionCache:

                throw new IllegalStateException("Session " + id + " already in cache");

which causes the catch block to activate in the above mentioned newHttpSession() function, which causes the session to return null. Further, getSession then passes this null session to the session cookie function, which throws the NPE exception. This might be easily reproducible if you were to extend the SessionHandler and then always return null for newHttpSession().

[janbartel]

I agree with you. Ideally I think SessionHandler.newHttpSession(Request) should throw Exception. That's a bit difficult to do in jetty-9.4 as it changes the method signature, but it can be done in jetty-10. For jetty-9.4 maybe I could throw IllegalStateException in Request.getSession(boolean).