On 05/12/2016 13:40, Péter Gergely Horváth wrote:
> 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.
In theory, it is a problem.
In practise, I'd be surprised if anyone was ever troubled by it.
Since it appears you did hit this issue, open a bug report and we'll get
Tomcat's implementation fixed.
Mark
>
> Thanks,
> Peter
>
>
> On Mon, Nov 28, 2016 at 6:45 PM, Christopher Schultz <
> ch...@christopherschultz.net> wrote:
>
> 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
>>>>>
>>>>>
>>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
>> For additional commands, e-mail: users-h...@tomcat.apache.org
>>
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org
