Review ServletHolder.getServlet

[gregw]

Jetty version
9.4.x

See discussion on c40b955#commitcomment-43538925

It appears that dropwizard has wrapped the holder class to avoid the synchronisation in ServletHolder.getServlet(). The class uses both a volatile _servlet field and synchronisation, yet always grabs the lock in getServlet(). We should either check the volatile before locking or not use a volatile at all. Perhaps the following implementation may avoid any contention that was seen on that lock:

    public Servlet getServlet()
        throws ServletException
    {
        Servlet servlet = _servlet;
        if (servlet == null)
        {
            synchronized (this)
            {
                if (_servlet == null && isRunning())
                {
                    if (getHeldClass() != null)
                        initServlet();
                }
                servlet = _servlet;
            }
        }

        return servlet;
    }

[gregw]

That suggestion won't work as it will allows lazy init servlets to be seen before they are initialized.
It needs to be something like:

    public Servlet getServlet()
        throws ServletException
    {
        if (_initOrder > 0)
        {
            Servlet servlet = _servlet;
            if (servlet != null)
                return servlet;
        }
        synchronized (this)
        {
            if (_servlet == null && isRunning())
            {
                if (getHeldClass() != null)
                    initServlet();
            }
        }

        return _servlet;
    }

at which point I'm thinking we really need to see evidence of contention.

[gregw]

@lorban @sbordet thoughts?

[lorban]

As you said, I would not touch this without some evidence of contention.

[gregw]

@janbartel has suggested to me that we could reduce lock contention by calling getServlet only once per execution in prepare and then use getServletInstance in handle.
I've also noticed that the initServlet method assignes the _servlet field and then initialises it.... which creates the potential for leaking an uninitialised servlet instance. Some of the changes from @joakime recent wrapping changes come into play here as well, as we can see that we inconsistently wrap the deprecated single threaded servlet.

So we've both worked on a clean up... which in the end looks like it makes the simpler version I first proposed workable. I think the cleanup is worth it for cleanup sake... but we can review if we really need the volatile once the clean up is reviewed.

Stand by for a cleanup PR....