Re: [xwiki-notifications] r24576 - in platform/core/trunk/xwiki-webdav/src/main/java/com/xpn/xwiki/plugin/webdav: . resources/domain resources/partial resources/views resources/views/attachments resources/views/pages

[Asiri Rathnayake]

Hi,

> ---
> platform/core/trunk/xwiki-webdav/src/main/java/com/xpn/xwiki/plugin/webdav/XWikiDavServlet.java
>     2009-10-20 11:50:17 UTC (rev 24575)
> +++
> platform/core/trunk/xwiki-webdav/src/main/java/com/xpn/xwiki/plugin/webdav/XWikiDavServlet.java
>     2009-10-20 11:51:34 UTC (rev 24576)
> @@ -74,17 +74,17 @@
>     /**
>      * Locator factory. {@link DavLocatorFactory}
>      */
> -    private DavLocatorFactory locatorFactory;
> +    private transient DavLocatorFactory locatorFactory;
>
>     /**
>      * Resource factory. {@link DavResourceFactory}
>      */
> -    private DavResourceFactory resourceFactory;
> +    private transient DavResourceFactory resourceFactory;
>
>     /**
>      * Session provider. {@link DavSessionProvider}
>      */
> -    private DavSessionProvider sessionProvider;
> +    private transient DavSessionProvider sessionProvider;
>

This might be a bad way to fix the findbugs errors. I just used it to see if
it works and committed without second thoughts. Any ideas on how to fix the
"mutable servlet field" error in a better way?

Thanks.

- Asiri
_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs

[vmassol]

On Oct 20, 2009, at 2:02 PM, Asiri Rathnayake wrote:

> Hi,
>
>
>
>> ---
>> platform/core/trunk/xwiki-webdav/src/main/java/com/xpn/xwiki/plugin/
>> webdav/XWikiDavServlet.java
>>    2009-10-20 11:50:17 UTC (rev 24575)
>> +++
>> platform/core/trunk/xwiki-webdav/src/main/java/com/xpn/xwiki/plugin/
>> webdav/XWikiDavServlet.java
>>    2009-10-20 11:51:34 UTC (rev 24576)
>> @@ -74,17 +74,17 @@
>>    /**
>>     * Locator factory. {@link DavLocatorFactory}
>>     */
>> -    private DavLocatorFactory locatorFactory;
>> +    private transient DavLocatorFactory locatorFactory;
>>
>>    /**
>>     * Resource factory. {@link DavResourceFactory}
>>     */
>> -    private DavResourceFactory resourceFactory;
>> +    private transient DavResourceFactory resourceFactory;
>>
>>    /**
>>     * Session provider. {@link DavSessionProvider}
>>     */
>> -    private DavSessionProvider sessionProvider;
>> +    private transient DavSessionProvider sessionProvider;
>>
>
> This might be a bad way to fix the findbugs errors. I just used it  
> to see if
> it works and committed without second thoughts. Any ideas on how to  
> fix the
> "mutable servlet field" error in a better way?

I don't understand why you've used "transient". Can you explain?

The findbugs descriptions are avail here:
http://findbugs.sourceforge.net/bugDescriptions.html

"
MSF: Mutable servlet field (MSF_MUTABLE_SERVLET_FIELD)
A web server generally only creates one instance of servlet or jsp  
class (i.e., treats the class as a Singleton), and will have multiple  
threads invoke methods on that instance to service multiple  
simultaneous requests. Thus, having a mutable instance field generally  
creates race conditions.

"

Thus IMO we just need to show that we don't change these field values  
over the lifetime of the Servlet and thus use the "final" keyword  
(with some javadoc comment explaining why obviously!).

Thanks

-Vincent
_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs

[Asiri Rathnayake]

Hi,


I don't understand why you've used "transient". Can you explain?
>

Sorry, "mutable servlet field" was not the error. It was "A non-serializable
value is stored into a non-transient field of a serializable class". My
mistake.

This is why I added the transient field thinking it would fix the problem.
It did fix the problem but I wasn't sure if that is the correct thing to do.

Thanks.

- Asiri
_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs

[vmassol]

On Oct 20, 2009, at 2:16 PM, Asiri Rathnayake wrote:

> Hi,
>
>
> I don't understand why you've used "transient". Can you explain?
>>
>
> Sorry, "mutable servlet field" was not the error. It was "A non-
> serializable
> value is stored into a non-transient field of a serializable class".  
> My
> mistake.
>
> This is why I added the transient field thinking it would fix the  
> problem.
> It did fix the problem but I wasn't sure if that is the correct  
> thing to do.

ok that's better indeed. The reason is that according to the spec a  
Servlet can be serialized (saved on disk) if the memory becomes low  
and thus must be able to be loaded back into memory at a later point  
(the same need for load-balancing).

However your change has introduced another problem: transient means  
the field will not be saved when the servlet is serialized. Thus when  
it's deserialized these fields will be null and the code will fail...

Whereas before the servlet would have failed on serialization, it'll  
now fail on deserialization...

Note: Deserialization will not call the constructor.

Solution: either make the field objects serializable or capture  
deserialization to set up the transient field values. Note that this  
last solution may potentially cause problems with threading so this  
might need to be synchronized I think. The best solution is to make  
the field Objects Serializable IMO.

WDYT?

Thanks
-Vincent


_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs

[Sergiu Dumitriu-2]

On 10/20/2009 02:16 PM, Asiri Rathnayake wrote:

> Hi,
>
>
> I don't understand why you've used "transient". Can you explain?
>>
>
> Sorry, "mutable servlet field" was not the error. It was "A non-serializable
> value is stored into a non-transient field of a serializable class". My
> mistake.
>
> This is why I added the transient field thinking it would fix the problem.
> It did fix the problem but I wasn't sure if that is the correct thing to do.
>
> Thanks.

Why don't you make them static? Instance members of a singleton class is
roughly equivalent to static members.

--
Sergiu Dumitriu
http://purl.org/net/sergiu/
_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs

[vmassol]

On Oct 20, 2009, at 2:32 PM, Sergiu Dumitriu wrote:

> On 10/20/2009 02:16 PM, Asiri Rathnayake wrote:
>> Hi,
>>
>>
>> I don't understand why you've used "transient". Can you explain?
>>>
>>
>> Sorry, "mutable servlet field" was not the error. It was "A non-
>> serializable
>> value is stored into a non-transient field of a serializable  
>> class". My
>> mistake.
>>
>> This is why I added the transient field thinking it would fix the  
>> problem.
>> It did fix the problem but I wasn't sure if that is the correct  
>> thing to do.
>>
>> Thanks.
>
> Why don't you make them static? Instance members of a singleton  
> class is
> roughly equivalent to static members.

Please don't use statics. This is an antipattern. BTW I'm pretty sure  
I've read that statics can be lost when an object is serialized if the  
JVM unloads the class.

Thanks
-Vincent
_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs

[Asiri Rathnayake]

Hi,


ok that's better indeed. The reason is that according to the spec a

> Servlet can be serialized (saved on disk) if the memory becomes low
> and thus must be able to be loaded back into memory at a later point
> (the same need for load-balancing).
>
> However your change has introduced another problem: transient means
> the field will not be saved when the servlet is serialized. Thus when
> it's deserialized these fields will be null and the code will fail...
>
> Whereas before the servlet would have failed on serialization, it'll
> now fail on deserialization...
>
> Note: Deserialization will not call the constructor.
>

I was hoping upon de serialization it will call init() where i initialize
these variables. But I can't find a reference that says so (yet).



> Solution: either make the field objects serializable or capture
> deserialization to set up the transient field values. Note that this
> last solution may potentially cause problems with threading so this
> might need to be synchronized I think. The best solution is to make
> the field Objects Serializable IMO.
>

One instance variable used is provided by jackrabbit library which is not
serializable. I can do a custom serialization or,

I can create these instance variable for each request (get***) which is a
waste.

Looking around to see if there is another way.

Thanks.

- Asiri

>
> WDYT?
>
> Thanks
> -Vincent
>
>
> _______________________________________________
> devs mailing list
> [hidden email]
> http://lists.xwiki.org/mailman/listinfo/devs
>

_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs

[vmassol]

IMO the problem is the design of XWikiDavServlet.
It shouldn't contain any field.

For example why not use the component manager to load a component to  
perform the handling of DAV requests?

The CM would be retrieved from the servlet context.

Thanks
-Vincent

On Oct 20, 2009, at 2:36 PM, Asiri Rathnayake wrote:

> Hi,
>
>
> ok that's better indeed. The reason is that according to the spec a
>> Servlet can be serialized (saved on disk) if the memory becomes low
>> and thus must be able to be loaded back into memory at a later point
>> (the same need for load-balancing).
>>
>> However your change has introduced another problem: transient means
>> the field will not be saved when the servlet is serialized. Thus when
>> it's deserialized these fields will be null and the code will fail...
>>
>> Whereas before the servlet would have failed on serialization, it'll
>> now fail on deserialization...
>>
>> Note: Deserialization will not call the constructor.
>>
>
> I was hoping upon de serialization it will call init() where i  
> initialize
> these variables. But I can't find a reference that says so (yet).
>
>
>
>> Solution: either make the field objects serializable or capture
>> deserialization to set up the transient field values. Note that this
>> last solution may potentially cause problems with threading so this
>> might need to be synchronized I think. The best solution is to make
>> the field Objects Serializable IMO.
>>
>
> One instance variable used is provided by jackrabbit library which  
> is not
> serializable. I can do a custom serialization or,
>
> I can create these instance variable for each request (get***) which  
> is a
> waste.
>
> Looking around to see if there is another way.
>
> Thanks.
>
> - Asiri
>
>
>>
>> WDYT?
>>
>> Thanks
>> -Vincent
>>
>>
>> _______________________________________________
>> devs mailing list
>> [hidden email]
>> http://lists.xwiki.org/mailman/listinfo/devs
>>
> _______________________________________________
> devs mailing list
> [hidden email]
> http://lists.xwiki.org/mailman/listinfo/devs

_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs

[Asiri Rathnayake]

Hi Vincent,

On Tue, Oct 20, 2009 at 6:18 PM, Vincent Massol <[hidden email]> wrote:

> IMO the problem is the design of XWikiDavServlet.
> It shouldn't contain any field.
>
> For example why not use the component manager to load a component to
> perform the handling of DAV requests?
>


jackrabbit DavServlet has following abstract methods:

<code>
 /**
     * Checks if the precondition for this request and resource is valid.
     *
     * @param request
     * @param resource
     * @return
     */
    abstract protected boolean isPreconditionValid(WebdavRequest request,
DavResource resource);

    /**
     * Returns the <code>DavSessionProvider</code>.
     *
     * @return the session provider
     */
    abstract public DavSessionProvider getDavSessionProvider();

    /**
     * Returns the <code>DavSessionProvider</code>.
     *
     * @param davSessionProvider
     */
    abstract public void setDavSessionProvider(DavSessionProvider
davSessionProvider);

    /**
     * Returns the <code>DavLocatorFactory</code>.
     *
     * @return the locator factory
     */
    abstract public DavLocatorFactory getLocatorFactory();

    /**
     * Sets the <code>DavLocatorFactory</code>.
     *
     * @param locatorFactory
     */
    abstract public void setLocatorFactory(DavLocatorFactory
locatorFactory);

    /**
     * Returns the <code>DavResourceFactory</code>.
     *
     * @return the resource factory
     */
    abstract public DavResourceFactory getResourceFactory();

    /**
     * Sets the <code>DavResourceFactory</code>.
     *
     * @param resourceFactory
     */
    abstract public void setResourceFactory(DavResourceFactory
resourceFactory);

    /**
     * Returns the value of the 'WWW-Authenticate' header, that is returned
in
     * case of 401 error.
     *
     * @return value of the 'WWW-Authenticate' header
     */
    abstract public String getAuthenticateHeaderValue();
</code>

Now I have few doubts about why there are setXXX() methods but the point is
we need those instance variables if we are to implement these methods.

Thanks.

- Asiri

> The CM would be retrieved from the servlet context.
>
> Thanks
> -Vincent
>
> On Oct 20, 2009, at 2:36 PM, Asiri Rathnayake wrote:
>
> > Hi,
> >
> >
> > ok that's better indeed. The reason is that according to the spec a
> >> Servlet can be serialized (saved on disk) if the memory becomes low
> >> and thus must be able to be loaded back into memory at a later point
> >> (the same need for load-balancing).
> >>
> >> However your change has introduced another problem: transient means
> >> the field will not be saved when the servlet is serialized. Thus when
> >> it's deserialized these fields will be null and the code will fail...
> >>
> >> Whereas before the servlet would have failed on serialization, it'll
> >> now fail on deserialization...
> >>
> >> Note: Deserialization will not call the constructor.
> >>
> >
> > I was hoping upon de serialization it will call init() where i
> > initialize
> > these variables. But I can't find a reference that says so (yet).
> >
> >
> >
> >> Solution: either make the field objects serializable or capture
> >> deserialization to set up the transient field values. Note that this
> >> last solution may potentially cause problems with threading so this
> >> might need to be synchronized I think. The best solution is to make
> >> the field Objects Serializable IMO.
> >>
> >
> > One instance variable used is provided by jackrabbit library which
> > is not
> > serializable. I can do a custom serialization or,
> >
> > I can create these instance variable for each request (get***) which
> > is a
> > waste.
> >
> > Looking around to see if there is another way.
> >
> > Thanks.
> >
> > - Asiri
> >
> >
> >>
> >> WDYT?
> >>
> >> Thanks
> >> -Vincent
> >>
> >>
> >> _______________________________________________
> >> devs mailing list
> >> [hidden email]
> >> http://lists.xwiki.org/mailman/listinfo/devs
> >>
> > _______________________________________________
> > devs mailing list
> > [hidden email]
> > http://lists.xwiki.org/mailman/listinfo/devs
>
> _______________________________________________
> devs mailing list
> [hidden email]
> http://lists.xwiki.org/mailman/listinfo/devs
>

_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs

[vmassol]

On Oct 20, 2009, at 3:04 PM, Asiri Rathnayake wrote:

> Hi Vincent,
>
> On Tue, Oct 20, 2009 at 6:18 PM, Vincent Massol <[hidden email]>  
> wrote:
>
>> IMO the problem is the design of XWikiDavServlet.
>> It shouldn't contain any field.
>>
>> For example why not use the component manager to load a component to
>> perform the handling of DAV requests?
>>
>
>
> jackrabbit DavServlet has following abstract methods:
>
> <code>
> /**
>     * Checks if the precondition for this request and resource is  
> valid.
>     *
>     * @param request
>     * @param resource
>     * @return
>     */
>    abstract protected boolean isPreconditionValid(WebdavRequest  
> request,
> DavResource resource);
>
>    /**
>     * Returns the <code>DavSessionProvider</code>.
>     *
>     * @return the session provider
>     */
>    abstract public DavSessionProvider getDavSessionProvider();
>
>    /**
>     * Returns the <code>DavSessionProvider</code>.
>     *
>     * @param davSessionProvider
>     */
>    abstract public void setDavSessionProvider(DavSessionProvider
> davSessionProvider);
>
>    /**
>     * Returns the <code>DavLocatorFactory</code>.
>     *
>     * @return the locator factory
>     */
>    abstract public DavLocatorFactory getLocatorFactory();
>
>    /**
>     * Sets the <code>DavLocatorFactory</code>.
>     *
>     * @param locatorFactory
>     */
>    abstract public void setLocatorFactory(DavLocatorFactory
> locatorFactory);
>
>    /**
>     * Returns the <code>DavResourceFactory</code>.
>     *
>     * @return the resource factory
>     */
>    abstract public DavResourceFactory getResourceFactory();
>
>    /**
>     * Sets the <code>DavResourceFactory</code>.
>     *
>     * @param resourceFactory
>     */
>    abstract public void setResourceFactory(DavResourceFactory
> resourceFactory);
>
>    /**
>     * Returns the value of the 'WWW-Authenticate' header, that is  
> returned
> in
>     * case of 401 error.
>     *
>     * @return value of the 'WWW-Authenticate' header
>     */
>    abstract public String getAuthenticateHeaderValue();
> </code>
>
> Now I have few doubts about why there are setXXX() methods but the  
> point is
> we need those instance variables if we are to implement these methods.

We could still either make them components and look them up in  
getXXX() or have a manager component that returns them.

Thanks
-Vincent

>
> Thanks.
>
> - Asiri
>
>
>
>> The CM would be retrieved from the servlet context.
>>
>> Thanks
>> -Vincent
>>
>> On Oct 20, 2009, at 2:36 PM, Asiri Rathnayake wrote:
>>
>>> Hi,
>>>
>>>
>>> ok that's better indeed. The reason is that according to the spec a
>>>> Servlet can be serialized (saved on disk) if the memory becomes low
>>>> and thus must be able to be loaded back into memory at a later  
>>>> point
>>>> (the same need for load-balancing).
>>>>
>>>> However your change has introduced another problem: transient means
>>>> the field will not be saved when the servlet is serialized. Thus  
>>>> when
>>>> it's deserialized these fields will be null and the code will  
>>>> fail...
>>>>
>>>> Whereas before the servlet would have failed on serialization,  
>>>> it'll
>>>> now fail on deserialization...
>>>>
>>>> Note: Deserialization will not call the constructor.
>>>>
>>>
>>> I was hoping upon de serialization it will call init() where i
>>> initialize
>>> these variables. But I can't find a reference that says so (yet).
>>>
>>>
>>>
>>>> Solution: either make the field objects serializable or capture
>>>> deserialization to set up the transient field values. Note that  
>>>> this
>>>> last solution may potentially cause problems with threading so this
>>>> might need to be synchronized I think. The best solution is to make
>>>> the field Objects Serializable IMO.
>>>>
>>>
>>> One instance variable used is provided by jackrabbit library which
>>> is not
>>> serializable. I can do a custom serialization or,
>>>
>>> I can create these instance variable for each request (get***) which
>>> is a
>>> waste.
>>>
>>> Looking around to see if there is another way.
>>>
>>> Thanks.
>>>
>>> - Asiri
>>>
>>>
>>>>
>>>> WDYT?
>>>>
>>>> Thanks
>>>> -Vincent

_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs