Hi Chris,
Thanks your four input: this question is somewhere in-between... :)
We have *definitely* seen cases, where a piece of code like the one below
sometimes (a couple of times from tens of thousands of successfully
serviced requests) found the stored field to be null - with a
NullPointerException being thrown on the first access of the fooBarService
field.
@WebServlet("/open")
public class FooBarServlet extends HttpServlet {
private FooBarService fooBarService;
@Override
public void init() throws ServletException {
try {
ApplicationContext applicationContext =
(ApplicationContext)
getServletContext().getAttribute("springContext");
fooBarService =
applicationContext.getBean("fooBarService",
FooBarService.class);
} catch (Exception ex) {
// wrap any exceptions to ServletException so as to comply with
Servlet contract
throw new ServletException("Initialization failed with
exception", ex);
}
}
protected void doGet(HttpServletRequest request, HttpServletResponse
response) throws ServletException, IOException {
processRequest(request, response);
}
protected void doPost(HttpServletRequest request, HttpServletResponse
response) throws ServletException, IOException {
processRequest(request, response);
}
private void processRequest(HttpServletRequest request,
HttpServletResponse response) throws IOException, ServletException {
List<FooBar> = fooBarService.getFooBars(); /// we have seen NPEs
thrown from here
/// ...
}
For the first glance, it seemed to be obvious that it must have been a
threading issue, which could easily be solved by adding volatile to the
stored fooBarService field.
However, I was more than confused seeing
that javax.servlet.GenericServlet#config uses the same approach by simply
storing the ServletConfig into a field and reading it later on without any
locking etc.
I quickly scanned through the Servlet specs and Catalina codes, but cannot
really locate any explicit reference to thread-safety of
javax.servlet.GenericServlet#getServletConfig, that is where the question
originates.
I would by much obliged if you had any inputs, ideas regarding this, or on
the approach one could take to confirm it on his/her own.
Thanks,
Peter
On Mon, Nov 28, 2016 at 6:45 PM, Christopher Schultz <
ch...@christopherschultz.net> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> Péter,
>
> On 11/28/16 11:01 AM, Péter Gergely Horváth wrote:
> > Thank you very much for your feedback, but I think there is a
> > slight misunderstanding here: I know the order in which a servlet
> > is initialized and put into service, and this question is not
> > related to that. It's related to the _visibility_ of the field.
> >
> > The question is about the thread safety of the field access: if
> > you carefully check the details of
> > javax.servlet.GenericServlet#config and compare the implementation
> > to the sample "NoVisibility" from the book Java Concurrency in
> > Practice, then it is seems to follow the same pattern, which is
> > "not thread-safe because the value field is accessed from both get
> > and set without synchronization. Among other hazards, it is
> > susceptible to stale values: if one thread calls set, other threads
> > calling get may or may not see that update." [1].
> >
> > Personally, I would like to understand why this implementation is
> > not (if not) susceptible to the stale values phenomenon [2]; there
> > might be someone, who can point me to the right direction.
>
> I think you are correct that there is a theoretical multi-threaded
> race-condition, here. The container may initialize the servlet in one
> thread and service e.g. the first request in another thread, and the
> ServletContext reference might not be written to main memory and then
> read-back by the request-processing thread.
>
> Potential fixes for this would be either to define the ServletContext
> member to be volatile or to use synchronized methods.'
>
> Adding synchronization to the init() method would not be a problem at
> all: there is very little monitor contention at that stage and the
> container would only expect to call that method a single time. Adding
> synchronization to the getServletContext method, though, might
> represent a reduction in performance, since getServletContext gets
> called many times during the lifetime of a Servlet, and from many thread
> s.
>
> Likewise, marking the ServletContext member as "volatile" would
> necessarily require a slow main-memory read every time
> getServletContext is called.
>
> I suspect simple analysis above is the reason for no multithreaded
> protection being placed on the ServletContext member in question.
>
> Péter, is this an academic discussion, or have you in fact seen a case
> where a servlet has been initialized and yet the first thread to
> process a request receives a null value when calling getServletContext?
>
> - -chris
>
> > On Mon, Nov 28, 2016 at 6:08 AM, andreas <andr...@junius.info>
> > wrote:
> >
> >> ---- On Fri, 25 Nov 2016 23:02:00 +0930 Péter Gergely Horváth
> >> wrote ----
> >>> Hi All,
> >>>
> >>> I am wondering why calling
> >>> javax.servlet.Servlet#getServletConfig() is thread safe: if you
> >>> check the implementation in javax.servlet.GenericServlet from
> >>> servlet API 3.0.1, you see the
> >> following:
> >>>
> >>> package javax.servlet;
> >>>
> >>> // lines omitted
> >>>
> >>> public abstract class GenericServlet implements Servlet,
> >>> ServletConfig, java.io.Serializable { // lines omitted
> >>>
> >>> private transient ServletConfig config;
> >>>
> >>> // lines omitted
> >>>
> >>> public void init(ServletConfig config) throws ServletException
> >>> { this.config = config; this.init(); }
> >>>
> >>> // lines omitted
> >>>
> >>> public ServletConfig getServletConfig() { return config; } //
> >>> lines omitted }
> >>>
> >>>
> >>> The field config is written in init(ServletConfig) without any
> >>> synchronization, locking or config being volatile, while
> >> getServletConfig()
> >>> could be called anytime from any later worker thread of the
> >>> servlet container. I took a quick look into Tomcat/Catalina
> >>> code base, however I see no indication of the servlet container
> >>> performing any magic, which would guarantee thread safe access
> >>> to field config through getServletConfig() from e.g.
> >>> javax.servlet.GenericServlet#service(ServletRequest,
> >>> ServletResponse) and the corresponding methods in
> >>> javax.servlet.http.HttpServlet.
> >>>
> >>> Am I missing something here? What will guarantee that if Thread
> >>> A is used to init(ServletConfig) then Thread B calling
> >>> getServletConfig() will see the correct state of the field
> >>> config (Java "happens-before"), and not
> >> e.g.
> >>> null?
> >>>
> >>> I am asking this because I have seen some legacy code, where a
> >>> servlet stored something into a field inside the init() method,
> >>> which was then
> >> used
> >>> from within a javax.servlet.http.HttpServlet#doGet or
> >>> javax.servlet.http.HttpServlet#doPost method, without
> >>> synchronization of any kind like this:
> >>>
> >>> public class FoobarServlet extends HttpServlet {
> >>>
> >>> private FoobarService foobarService;
> >>>
> >>> @Override public void init() throws ServletException {
> >>> this.foobarService = // ... acquire foobarService } protected
> >>> void doGet(HttpServletRequest request, HttpServletResponse
> >>> response) throws ServletException, IOException { List<Foobar>
> >>> foobars = this.foobarService.getFoobars(); // read the field
> >>> WITHOUT synchronization // ... } // ... Is this approach
> >>> (having no synchronization, locking or the field being
> >>> volatile) correct? I assumed it is not, seeing something like
> >>> that in the servlet API is quite confusing.
> >>>
> >>>
> >>> What do you think?
> >>>
> >>> Thanks, Peter
> >>
> >>
> >> A Servlet will process requests only if it is fully initialized,
> >> i.e. init has been executed. The init method gets called only
> >> once and the servlet config won't change afterwards. I don't
> >> think there is need for synchronization. The same is probably
> >> valid for your own objects. Problems arise when individual
> >> requests change the state of these objects.
> >>
> >> Andy
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >> ---------------------------------------------------------------------
> >>
> >>
> To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
> >> For additional commands, e-mail: users-h...@tomcat.apache.org
> >>
> >>
> >
> -----BEGIN PGP SIGNATURE-----
> Comment: GPGTools - http://gpgtools.org
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iQIcBAEBCAAGBQJYPG0dAAoJEBzwKT+lPKRYkLUP/3JrIPCjIsmw4o71nmZUeN4r
> 0rt1ccflgjFUmpff5Pt9kHWJR60tga+sFT0jQ7LEvggbua1SoXV4y4MDINgEQ/Uu
> ek0aiDRR0n8xXxUWcx7P8Jjmn5yyOGUrFVw029dD55XT6ArYz7gaAUnowRKB6DR+
> RT84yO14TlRZx8DDLoxegmUpmXDpoHZyPKGGYWDhNepiU00X1YmFg/G3CiXJEjzd
> B3GBewhE8nh9GZsqfg7U1V9EBQI+8jeI/GxRTSnsUwWDKvH7Ja6ffiLtAS+3mnWD
> Ad2/wTbY7RT9ybT+thgvXkKJEKx+rVZrazb7QSgqq8Otv/aeMVLh16z2sxt6uuMy
> fffKRwNUL12cKr8KdNvUnb4PVStVF8fnRxAW1M90axzFvxeoLOJJZXtS6TBy+MjE
> eeMbjFV2wJw2y+66YWrvyCglyQU4c2ab5H0JuBINJ5oD1DSdTks0dW2rZvIm6pYp
> kCQTLSbZFjePksoNw6gxDo5XQW1SczWbTG2lYwEjfm3eeyHRnaRgiV4LPBPsx5Zy
> 7QFZJVGSFug2pgKTKErugoQs8z4gFCrryvebbGYgtvlsdJRk5Ldnl6uG0KabAEVU
> PedppbSTPPkE8Ym+/YTTpJ4Bo5NMjkoYySWnWpBLof5sJ1hkjkURc4cBiFw4vqZO
> K7WcPH4LDk5J5HzFiTIS
> =DLPe
> -----END PGP SIGNATURE-----
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: users-h...@tomcat.apache.org
>
>
