On Thu, Apr 13, 2017 at 8:45 AM, Shing Wai Chan
<shing.wai.chan_at_oracle.com> wrote:
>
>> On Apr 12, 2017, at 2:48 PM, Stuart Douglas <sdouglas_at_redhat.com> wrote:
>>
>> On Thu, Apr 13, 2017 at 3:46 AM, Shing Wai Chan
>> <shing.wai.chan_at_oracle.com> wrote:
>>>
>>>> On Apr 11, 2017, at 6:30 PM, Stuart Douglas <sdouglas_at_redhat.com> wrote:
>>>>
>>>> It is up to the Servlet container to ensure there is a happens-before
>>>> relationship between init() finishing and service() being called. Even
>>>> though the spec actually just says 'before' and does not explicitly
>>>> state that it is a 'happens-before' relationship as per JSR-133 I
>>>> think that this is really the only way you can define 'before' with
>>>> regards to multiple threads on the JVM.
>>>
>>> I agree that the `before’ seems to imply ‘happen-before’ here.
>>> The issue 81 is asking for clarification of “before” as “happen-before”.
>>>
>>>>
>>>> I examined the implementations of Undertow, Jetty, Tomcat and
>>>> Glassfish and it appears that they all use the synchronized keyword to
>>>> achieve this, in some cases combined with double checked locking to
>>>> minimize the performance impact (so the lock will only be taken while
>>>> init is running, otherwise the only cost is a volatile read).
>>>
>>> I have took a quick look on some of the above implementations.
>>> It seems that they have synchronization for creation of servlet.
>>> But I am not sure about the invocation of servlet#service there.
>>>
>>
>> You don't need to synchronize on the invocation of servlet#service
>> (which would be a bad idea for obvious reasons). You just need some
>> kind of memory barrier when you acquire the Servlet instance to invoke
>> upon. Some implementations seem to use a straight synchronization,
>> while others use synchronization combined with a volatile (double
>> checked locking). Either way because this happens before the service
>> method is invoked in the same thread there is a happens before
>> relationship between the sync/volatile and invoking the service
>> method, and the sync is coded to make sure there is a happens before
>> between invoking the init method and the servlet being acquired.
>
> Let me recap what we discuss.
> We have
> B - The init method completing
> C - The service method begin invoked
> we would like to have hb(B, C).
> From 17.4.5 Happens-before Order, we need to have
> (ii) B synchronizes-with a following action C
> where synchronizes-with is defined in 17.4.4 which may include lock, volatile, etc.
> I would like to clarify that some works (more than just synchronize the servlet creation; possibly with volatile) need to be done in the container in order to have (ii).
>
> With this understanding, then we can go back to the question whether we want to put “happen-before” in the spec.
Yes, but it is the same work that needs to be done to ensure the init
method has completed before calling service() (which makes sense,
because it is basically the same thing).
If you think about it from a containers point of view when creating
and initializing a Servlet the basic requirements are:
1) Create and initialize it under some form of lock, so that it is not
created or initialized twice
2) Publish it in a thread safe manner so that other threads can use it
And basically there is no way to do step 2) without causing a
happens-before relationship between init() and the other thread
getting access to the created Servlet.
e.g. here is a very simple implementation:
private Servlet servlet;
synchronized Servlet getServlet() {
if(servlet == null) {
servlet = createServlet();
servlet.init(config);
}
return servlet;
}
void callServlet(ServletRequest req, ServletResponse resp) {
getServlet().service(req, resp);
}
This is all that is required to make sure that init() has a happens
before relationship with service(). A better implementation using
double checked locking would be:
private volatile Servlet servlet;
Servlet getServlet() {
if(servlet == null) {
synchronized (this) {
if(servlet == null) {
servlet = createServlet();
servlet.init(config);
}
}
}
return servlet;
}
void callServlet(ServletRequest req, ServletResponse resp) {
getServlet().service(req, resp);
}
As this avoids the lock once the Servlet has been created. Basically
any compliant container will already do this.
Stuart
>
> SD> 1) Do nothing and close the JIRA, with a note explaining that volatile
> SD> is not necessary, as it is not possible to satisfy the relationship
> SD> that init() happens before service() without also establishing a
> SD> happens-before relationship that means that any field changes will be
> SD> visible to the thread invoking the service() method.
>
> SD> 2) Clarify at the platform level that Java EE containers will ensure
> SD> there is a happens before relationship between the initialization of
> SD> any component that is used by multiple threads and its initialization.
> SD> The reason why I think that this should happen at the platform level
> SD> is that if we just clarify at the Servlet level people might assume
> SD> that because we explicitly specify the relationship and other
> SD> specification don't that they need to use volatile in other components
> SD> (e.g. JAX-RS endpoints).
>
> SD> 3) Change the spec to say that there must be a happens-before
> SD> relationship between init() and service() (possibly with a note that
> SD> just because this relationship is explicitly specified it does not
> SD> mean that you need to start worrying about happens-before in other
> SD> specifications that don't explicitly state this).
>
> Thanks.
> Shing Wai Chan
>
>>
>> Stuart
>>
>>>>
>>>> There are other ways to achieve this, namely by using classes from the
>>>> java.util.concurrent package (which provide their own happens before
>>>> guarantees, usually specified in the javadoc of the class).
>>>>
>>>> Stuart
>>>>
>>>>
>>>> On Wed, Apr 12, 2017 at 10:35 AM, Shing Wai Chan
>>>> <shing.wai.chan_at_oracle.com> wrote:
>>>>>
>>>>>> On Apr 11, 2017, at 4:43 PM, Stuart Douglas <sdouglas_at_redhat.com> wrote:
>>>>>>
>>>>>> On Wed, Apr 12, 2017 at 8:34 AM, Mark Thomas <markt_at_apache.org> wrote:
>>>>>>> On 11/04/17 16:42, Edward Burns wrote:
>>>>>>>>>>>>> On Fri, 24 Mar 2017 09:51:38 +0000, Mark Thomas <markt_at_apache.org> said:
>>>>>>>> SD> Concurrency is hard, and this is exactly the sort of concurrency issue
>>>>>>>> SD> that Java EE is supposed to solve, we should not be punting it onto
>>>>>>>> SD> application developers when it is trivial for us to make sure the
>>>>>>>> SD> containers do the correct thing.
>>>>>>>>
>>>>>>>> MT> +1
>>>>>>>>
>>>>>>>> MT> I couldn't agree more with everything Stuart has written.
>>>>>>>>
>>>>>>>>>>>>> On Mon, 10 Apr 2017 12:09:36 -0700, Edward Burns <edward.burns_at_oracle.com> said:
>>>>>>>>
>>>>>>>> EB> Shing-wai looked into the GlassFish implementation and suggested we
>>>>>>>> EB> simply leave the issue in the JIRA and look to resolve it in a future
>>>>>>>> EB> release. I will take this option.
>>>>>>>>
>>>>>>>> In case this was not clear, I also agree with this resolution of leaving
>>>>>>>> the issue in JIRA for 4.0.
>>>>>>>
>>>>>>> I'm not sure why this is being pushed out to the next spec version.
>>>>>>> Isn't this as simple as making the ServletConfig field in GenericServlet
>>>>>>> volatile?
>>>>>>
>>>>>> No, making fields volatile will not help at all.
>>>>>>
>>>>>> At the moment the spec says: "After the servlet object is
>>>>>> instantiated, the container must initialize the servlet before it can
>>>>>> handle requests from clients." It is not actually possible to meet
>>>>>> this condition without also establishing a happens-before relationship
>>>>>> that guarantees any writes from the init() method are visible to the
>>>>>> thread calling service() (see below for the logic behind this).
>>>>>>
>>>>>> This means adding volatile does nothing, as either the container
>>>>>> implements the specification and it is unnecessary, or it does not and
>>>>>> init() may not have finished, so volatile will not help as the write
>>>>>> may not have actually happened yet.
>>>>>>
>>>>>> IMHO we should do one of the following:
>>>>>>
>>>>>> 1) Do nothing and close the JIRA, with a note explaining that volatile
>>>>>> is not necessary, as it is not possible to satisfy the relationship
>>>>>> that init() happens before service() without also establishing a
>>>>>> happens-before relationship that means that any field changes will be
>>>>>> visible to the thread invoking the service() method.
>>>>>>
>>>>>> 2) Clarify at the platform level that Java EE containers will ensure
>>>>>> there is a happens before relationship between the initialization of
>>>>>> any component that is used by multiple threads and its initialization.
>>>>>> The reason why I think that this should happen at the platform level
>>>>>> is that if we just clarify at the Servlet level people might assume
>>>>>> that because we explicitly specify the relationship and other
>>>>>> specification don't that they need to use volatile in other components
>>>>>> (e.g. JAX-RS endpoints).
>>>>>>
>>>>>> 3) Change the spec to say that there must be a happens-before
>>>>>> relationship between init() and service() (possibly with a note that
>>>>>> just because this relationship is explicitly specified it does not
>>>>>> mean that you need to start worrying about happens-before in other
>>>>>> specifications that don't explicitly state this).
>>>>>>
>>>>>>
>>>>>> The formal Java Memory Model reasoning (see
>>>>>> http://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.4.5
>>>>>> )
>>>>>>
>>>>>> According to the model the two bits we are interested in is:
>>>>>>
>>>>>> (1) If x and y are actions of the same thread and x comes before y in
>>>>>> program order, then hb(x, y).
>>>>>> (2) If hb(x, y) and hb(y, z), then hb(x, z).
>>>>>>
>>>>>> And from the Servlet spec we have:
>>>>>>
>>>>>> (3) After the servlet object is instantiated, the container must
>>>>>> initialize the servlet before it can handle requests from clients.
>>>>>>
>>>>>> If we define the following actions:
>>>>>> A - The field write in the init method
>>>>>> B - The init method completing
>>>>>> C - The service method being invoked
>>>>>>
>>>>>> We know that we have the following:
>>>>>>
>>>>>> hb(A, B) - the field write has to happen before the init method
>>>>>> completes, as per (1)
>>>>>> hb(B, C) - init is finished before calling service(), as per (3)
>>>>>
>>>>> My question is on hb(B, C).
>>>>> From 17.4.5, we can have hb(B, C) if
>>>>> (i) B and C are on the same thread and B comes before C
>>>>> (ii) B synchronizes-with a following action C
>>>>> (iii) there is a D such that hb(B, D) and hb(D, C)
>>>>> (iv) B before Thread.start().
>>>>>
>>>>> Since B and C may be on different threads, we only have (ii) and (iii).
>>>>> ( (iv) may require using new thread for those requests. This may be not easy to do.)
>>>>> (ii) may request a synchronization/lock. Is it a concern here?
>>>>> Or can we find D and use (iii)?
>>>>>
>>>>> Thanks.
>>>>> Shing Wai Chan
>>>>>
>>>>>>
>>>>>> We can then infer:
>>>>>> hb(A, C) - the field write has to happen before the service method is
>>>>>> called, as per (2)
>>>>>>
>>>>>> Stuart
>>>>>>
>>>>>>>
>>>>>>> Mark
>>>>>>>
>>>>>
>>>
>