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

Review ServletHolder.getServlet #5498

Closed
gregw opened this issue Oct 25, 2020 · 4 comments
Closed

Review ServletHolder.getServlet #5498

gregw opened this issue Oct 25, 2020 · 4 comments
Assignees
Labels

Comments

@gregw
Copy link
Contributor

gregw commented Oct 25, 2020

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 gregw added the Question label Oct 25, 2020
gregw referenced this issue Oct 25, 2020
* Issue #5022 Filter Cache cleanup

Issue #5022 Filter Cache cleanup:
 + Fixed many compiler warnings
 + removed old LazyList leftovers
 + Don't create holder string for source unless required
 + Only have a single type of chain, so it can be wrapped regardless of cache
 + Reverse mappings lists to make filter chain creation easier
 + build chain directly rather than build a list then a chain

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* added comment to explain ordering

Signed-off-by: gregw <gregw@webtide.com>

* More cleanups

* fixed toString format
turn off debug in OSGI test
@gregw
Copy link
Contributor Author

gregw commented Oct 25, 2020

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
Copy link
Contributor Author

gregw commented Oct 25, 2020

@lorban @sbordet thoughts?

@lorban
Copy link
Contributor

lorban commented Oct 26, 2020

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

@gregw
Copy link
Contributor Author

gregw commented Oct 26, 2020

@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....

gregw added a commit that referenced this issue Oct 26, 2020
Various cleanups including:
 + renaming multiple `_servlet` fields in inner classes to avoid confusion
 + better comments in prepare method to describe why it is needed
 + call prepare from Invoker servlet
 + The `_servlet` field is not set until after the servlet is initialized
 + Consistent wrapping of `SingleThreadedWrapper` now in `initServlet`
 + The `getServlet` method now looks the volatile `_servlet` to avoid locking if possible
 + The `handle` method now calls `getServletInstance` as servlet will have been initialized in `prepare`
 + Found and fixed race with making unavaiable servlet available again
@gregw gregw closed this as completed in 7a0de6a Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants