XWikiDocuments not thread safe added to XWikiCacheStore

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

XWikiDocuments not thread safe added to XWikiCacheStore

Fabian Pichler
Hi devs

http://jira.xwiki.org/browse/XWIKI-13623
I would like to discuss the suggested solution, before I start implementing
it:

*I suggest the following solution:*
Introducing a nested DocumentLoader class which loads the document calling
the basestore loadXWikiDoc in a synchronized getDocOrLoad method. The
DocumentLoader objects gets instantiated in the cacheStore on a cache miss.
It then gets stored in a ConcurrentHashMap using the same key which is used
for the document cache (DocumentReference including language). Using
putIfAbsent to insert the DocumentLoader and calling the synchronized load
method on the object in the map solves the memory visibility issue and
moreover ensures that the same XWikiDocument is not loaded multiple times
concurrently from the database. It is still possible to load different
documents in parallel. A DocumentLoader will load the XWikiDocument only
once and the returns the identical document object on any following call of
the synchronized getDocOrLoad method.

WDYT?

Kind Regards,
Fabian
_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs
Reply | Threaded
Open this post in threaded view
|

Re: XWikiDocuments not thread safe added to XWikiCacheStore

Thomas Mortagne
Administrator
XWikiCacheStore#loadXWikiDoc is actually a bit tricker than that right
now for retro compatibility and laziness (really need to rewrite this
whole store API) reasons and before applying what you propose the way
it deals with document uid should be modified a bit.

Hibernate store is always loading the document from the wiki indicated
in the context completely ignoring the wiki in the XWikiDocument
object. That means that doc.getKey() may be different than the key of
the document that will actually come from the hibernate store. So
before adding DocumentLoader jingling a first thing to do is generate
the initial cache key with a mix of the wiki id located in the context
and the XWikiDocument reference.

Then what you propose sounds good.

On Mon, Aug 8, 2016 at 8:53 AM, Fabian Pichler
<[hidden email]> wrote:

> Hi devs
>
> http://jira.xwiki.org/browse/XWIKI-13623
> I would like to discuss the suggested solution, before I start implementing
> it:
>
> *I suggest the following solution:*
> Introducing a nested DocumentLoader class which loads the document calling
> the basestore loadXWikiDoc in a synchronized getDocOrLoad method. The
> DocumentLoader objects gets instantiated in the cacheStore on a cache miss.
> It then gets stored in a ConcurrentHashMap using the same key which is used
> for the document cache (DocumentReference including language). Using
> putIfAbsent to insert the DocumentLoader and calling the synchronized load
> method on the object in the map solves the memory visibility issue and
> moreover ensures that the same XWikiDocument is not loaded multiple times
> concurrently from the database. It is still possible to load different
> documents in parallel. A DocumentLoader will load the XWikiDocument only
> once and the returns the identical document object on any following call of
> the synchronized getDocOrLoad method.
>
> WDYT?
>
> Kind Regards,
> Fabian
> _______________________________________________
> devs mailing list
> [hidden email]
> http://lists.xwiki.org/mailman/listinfo/devs



--
Thomas Mortagne
_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs
Reply | Threaded
Open this post in threaded view
|

Re: XWikiDocuments not thread safe added to XWikiCacheStore

Fabian Pichler
Hi Thomas

Thanks for responding and thanks for supporting the suggested solution.

I understand the doc.Key-issue that you mentioned. Yet, I think this is not
a
XWikiCacheStore#loadXWikDoc concern. In my understanding the expected
document
to load is defined by the document-reference and the language. Thus the
Hibernate-Store must be concerned in setting the context database according
to the Wiki-Reference before loading the document. This is acctually already
done in the XWiki#getDocument call. I agree that this might be the wrong
place to do it.
Thus, I would suggest to fix this issue in the XWikiHibernateStore on a
seperate jira-issue.
Maybe this could be combined with a significant performance improvment in
loading
the documents with fewer sql-queries per loading action.

With XWIKI-13623 I try to tackle the issue about randomly loosing BaseObject
data because of an not thread safe construction and publication of
XWikiDocument
java objects in XWikiCacheStore. Because this issue gave us a big headache
in
production multiple times, I solved it in our project in the way mentioned.

If the xwiki-dev team supports the suggested solution, I would be willing
to proceed
and prepare a pull request for an adapted XWikiCacheStore fix.

Kind regards,
Fabian


2016-08-08 12:07 GMT+02:00 Thomas Mortagne <[hidden email]>:

> XWikiCacheStore#loadXWikiDoc is actually a bit tricker than that right
> now for retro compatibility and laziness (really need to rewrite this
> whole store API) reasons and before applying what you propose the way
> it deals with document uid should be modified a bit.
>
> Hibernate store is always loading the document from the wiki indicated
> in the context completely ignoring the wiki in the XWikiDocument
> object. That means that doc.getKey() may be different than the key of
> the document that will actually come from the hibernate store. So
> before adding DocumentLoader jingling a first thing to do is generate
> the initial cache key with a mix of the wiki id located in the context
> and the XWikiDocument reference.
>
> Then what you propose sounds good.
>
> On Mon, Aug 8, 2016 at 8:53 AM, Fabian Pichler
> <[hidden email]> wrote:
> > Hi devs
> >
> > http://jira.xwiki.org/browse/XWIKI-13623
> > I would like to discuss the suggested solution, before I start
> implementing
> > it:
> >
> > *I suggest the following solution:*
> > Introducing a nested DocumentLoader class which loads the document
> calling
> > the basestore loadXWikiDoc in a synchronized getDocOrLoad method. The
> > DocumentLoader objects gets instantiated in the cacheStore on a cache
> miss.
> > It then gets stored in a ConcurrentHashMap using the same key which is
> used
> > for the document cache (DocumentReference including language). Using
> > putIfAbsent to insert the DocumentLoader and calling the synchronized
> load
> > method on the object in the map solves the memory visibility issue and
> > moreover ensures that the same XWikiDocument is not loaded multiple times
> > concurrently from the database. It is still possible to load different
> > documents in parallel. A DocumentLoader will load the XWikiDocument only
> > once and the returns the identical document object on any following call
> of
> > the synchronized getDocOrLoad method.
> >
> > WDYT?
> >
> > Kind Regards,
> > Fabian
> > _______________________________________________
> > devs mailing list
> > [hidden email]
> > http://lists.xwiki.org/mailman/listinfo/devs
>
>
>
> --
> Thomas Mortagne
> _______________________________________________
> devs mailing list
> [hidden email]
> http://lists.xwiki.org/mailman/listinfo/devs
>
_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs
Reply | Threaded
Open this post in threaded view
|

Re: XWikiDocuments not thread safe added to XWikiCacheStore

Thomas Mortagne
Administrator
On Wed, Aug 10, 2016 at 10:14 AM, Fabian Pichler
<[hidden email]> wrote:

> Hi Thomas
>
> Thanks for responding and thanks for supporting the suggested solution.
>
> I understand the doc.Key-issue that you mentioned. Yet, I think this is not
> a
> XWikiCacheStore#loadXWikDoc concern. In my understanding the expected
> document
> to load is defined by the document-reference and the language. Thus the
> Hibernate-Store must be concerned in setting the context database according
> to the Wiki-Reference before loading the document. This is acctually already
> done in the XWiki#getDocument call. I agree that this might be the wrong
> place to do it.
> Thus, I would suggest to fix this issue in the XWikiHibernateStore on a
> seperate jira-issue.

In an ideal world it would be fixed in XWikiHibernateStore I agree but
this would break retro compatibility (yeah always a pain). That's why
I talked about providing a new API.

So right now in practice using a combination of context wiki and
document reference is part of the API definition and can be applied at
XWikiCacheStore too.

Note that in practice this is actually a pretty rare case since anyone
is supposed to use XWiki#getDocument which make sure the context
contain the document wiki. But this can still happen since this is a
very old and public API.

> Maybe this could be combined with a significant performance improvment in
> loading
> the documents with fewer sql-queries per loading action.
>
> With XWIKI-13623 I try to tackle the issue about randomly loosing BaseObject
> data because of an not thread safe construction and publication of
> XWikiDocument
> java objects in XWikiCacheStore. Because this issue gave us a big headache
> in
> production multiple times, I solved it in our project in the way mentioned.
>
> If the xwiki-dev team supports the suggested solution, I would be willing
> to proceed
> and prepare a pull request for an adapted XWikiCacheStore fix.
>
> Kind regards,
> Fabian
>
>
> 2016-08-08 12:07 GMT+02:00 Thomas Mortagne <[hidden email]>:
>
>> XWikiCacheStore#loadXWikiDoc is actually a bit tricker than that right
>> now for retro compatibility and laziness (really need to rewrite this
>> whole store API) reasons and before applying what you propose the way
>> it deals with document uid should be modified a bit.
>>
>> Hibernate store is always loading the document from the wiki indicated
>> in the context completely ignoring the wiki in the XWikiDocument
>> object. That means that doc.getKey() may be different than the key of
>> the document that will actually come from the hibernate store. So
>> before adding DocumentLoader jingling a first thing to do is generate
>> the initial cache key with a mix of the wiki id located in the context
>> and the XWikiDocument reference.
>>
>> Then what you propose sounds good.
>>
>> On Mon, Aug 8, 2016 at 8:53 AM, Fabian Pichler
>> <[hidden email]> wrote:
>> > Hi devs
>> >
>> > http://jira.xwiki.org/browse/XWIKI-13623
>> > I would like to discuss the suggested solution, before I start
>> implementing
>> > it:
>> >
>> > *I suggest the following solution:*
>> > Introducing a nested DocumentLoader class which loads the document
>> calling
>> > the basestore loadXWikiDoc in a synchronized getDocOrLoad method. The
>> > DocumentLoader objects gets instantiated in the cacheStore on a cache
>> miss.
>> > It then gets stored in a ConcurrentHashMap using the same key which is
>> used
>> > for the document cache (DocumentReference including language). Using
>> > putIfAbsent to insert the DocumentLoader and calling the synchronized
>> load
>> > method on the object in the map solves the memory visibility issue and
>> > moreover ensures that the same XWikiDocument is not loaded multiple times
>> > concurrently from the database. It is still possible to load different
>> > documents in parallel. A DocumentLoader will load the XWikiDocument only
>> > once and the returns the identical document object on any following call
>> of
>> > the synchronized getDocOrLoad method.
>> >
>> > WDYT?
>> >
>> > Kind Regards,
>> > Fabian
>> > _______________________________________________
>> > devs mailing list
>> > [hidden email]
>> > http://lists.xwiki.org/mailman/listinfo/devs
>>
>>
>>
>> --
>> Thomas Mortagne
>> _______________________________________________
>> devs mailing list
>> [hidden email]
>> http://lists.xwiki.org/mailman/listinfo/devs
>>
> _______________________________________________
> devs mailing list
> [hidden email]
> http://lists.xwiki.org/mailman/listinfo/devs



--
Thomas Mortagne
_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs
Reply | Threaded
Open this post in threaded view
|

Re: XWikiDocuments not thread safe added to XWikiCacheStore

Fabian Pichler
ok, if I understand correctly the API contract so far is, that the context
defines the database used and the reference of the document to load
must only be taken relative to the given context. (old String fullName
and context-database interpretation).

So, I think it would be better to compute the lookup key based on the
context database and the fullName computed out of the document reference.
Otherwise you would not get the same document in both situations.
1. from cache
2. freshly loaded

Anyway, I am not sure if it is a good idea to fix the key-issue as part of
XWIKI-13623. I think that XWIKI-13623 should only focus on the mentioned
memory visability and resulting data loss issue.

Fabian.

2016-08-10 10:52 GMT+02:00 Thomas Mortagne <[hidden email]>:

> On Wed, Aug 10, 2016 at 10:14 AM, Fabian Pichler
> <[hidden email]> wrote:
> > Hi Thomas
> >
> > Thanks for responding and thanks for supporting the suggested solution.
> >
> > I understand the doc.Key-issue that you mentioned. Yet, I think this is
> not
> > a
> > XWikiCacheStore#loadXWikDoc concern. In my understanding the expected
> > document
> > to load is defined by the document-reference and the language. Thus the
> > Hibernate-Store must be concerned in setting the context database
> according
> > to the Wiki-Reference before loading the document. This is acctually
> already
> > done in the XWiki#getDocument call. I agree that this might be the wrong
> > place to do it.
> > Thus, I would suggest to fix this issue in the XWikiHibernateStore on a
> > seperate jira-issue.
>
> In an ideal world it would be fixed in XWikiHibernateStore I agree but
> this would break retro compatibility (yeah always a pain). That's why
> I talked about providing a new API.
>
> So right now in practice using a combination of context wiki and
> document reference is part of the API definition and can be applied at
> XWikiCacheStore too.
>
> Note that in practice this is actually a pretty rare case since anyone
> is supposed to use XWiki#getDocument which make sure the context
> contain the document wiki. But this can still happen since this is a
> very old and public API.
>
> > Maybe this could be combined with a significant performance improvment in
> > loading
> > the documents with fewer sql-queries per loading action.
> >
> > With XWIKI-13623 I try to tackle the issue about randomly loosing
> BaseObject
> > data because of an not thread safe construction and publication of
> > XWikiDocument
> > java objects in XWikiCacheStore. Because this issue gave us a big
> headache
> > in
> > production multiple times, I solved it in our project in the way
> mentioned.
> >
> > If the xwiki-dev team supports the suggested solution, I would be willing
> > to proceed
> > and prepare a pull request for an adapted XWikiCacheStore fix.
> >
> > Kind regards,
> > Fabian
> >
> >
> > 2016-08-08 12:07 GMT+02:00 Thomas Mortagne <[hidden email]>:
> >
> >> XWikiCacheStore#loadXWikiDoc is actually a bit tricker than that right
> >> now for retro compatibility and laziness (really need to rewrite this
> >> whole store API) reasons and before applying what you propose the way
> >> it deals with document uid should be modified a bit.
> >>
> >> Hibernate store is always loading the document from the wiki indicated
> >> in the context completely ignoring the wiki in the XWikiDocument
> >> object. That means that doc.getKey() may be different than the key of
> >> the document that will actually come from the hibernate store. So
> >> before adding DocumentLoader jingling a first thing to do is generate
> >> the initial cache key with a mix of the wiki id located in the context
> >> and the XWikiDocument reference.
> >>
> >> Then what you propose sounds good.
> >>
> >> On Mon, Aug 8, 2016 at 8:53 AM, Fabian Pichler
> >> <[hidden email]> wrote:
> >> > Hi devs
> >> >
> >> > http://jira.xwiki.org/browse/XWIKI-13623
> >> > I would like to discuss the suggested solution, before I start
> >> implementing
> >> > it:
> >> >
> >> > *I suggest the following solution:*
> >> > Introducing a nested DocumentLoader class which loads the document
> >> calling
> >> > the basestore loadXWikiDoc in a synchronized getDocOrLoad method. The
> >> > DocumentLoader objects gets instantiated in the cacheStore on a cache
> >> miss.
> >> > It then gets stored in a ConcurrentHashMap using the same key which is
> >> used
> >> > for the document cache (DocumentReference including language). Using
> >> > putIfAbsent to insert the DocumentLoader and calling the synchronized
> >> load
> >> > method on the object in the map solves the memory visibility issue and
> >> > moreover ensures that the same XWikiDocument is not loaded multiple
> times
> >> > concurrently from the database. It is still possible to load different
> >> > documents in parallel. A DocumentLoader will load the XWikiDocument
> only
> >> > once and the returns the identical document object on any following
> call
> >> of
> >> > the synchronized getDocOrLoad method.
> >> >
> >> > WDYT?
> >> >
> >> > Kind Regards,
> >> > Fabian
> >> > _______________________________________________
> >> > devs mailing list
> >> > [hidden email]
> >> > http://lists.xwiki.org/mailman/listinfo/devs
> >>
> >>
> >>
> >> --
> >> Thomas Mortagne
> >> _______________________________________________
> >> devs mailing list
> >> [hidden email]
> >> http://lists.xwiki.org/mailman/listinfo/devs
> >>
> > _______________________________________________
> > devs mailing list
> > [hidden email]
> > http://lists.xwiki.org/mailman/listinfo/devs
>
>
>
> --
> Thomas Mortagne
> _______________________________________________
> devs mailing list
> [hidden email]
> http://lists.xwiki.org/mailman/listinfo/devs
>
_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs
Reply | Threaded
Open this post in threaded view
|

Re: XWikiDocuments not thread safe added to XWikiCacheStore

Thomas Mortagne
Administrator
On Wed, Aug 10, 2016 at 12:41 PM, Fabian Pichler
<[hidden email]> wrote:
> ok, if I understand correctly the API contract so far is, that the context
> defines the database used and the reference of the document to load
> must only be taken relative to the given context. (old String fullName
> and context-database interpretation).

Yes.

>
> So, I think it would be better to compute the lookup key based on the
> context database and the fullName computed out of the document reference.
> Otherwise you would not get the same document in both situations.
> 1. from cache
> 2. freshly loaded

Yes this what I explained.

>
> Anyway, I am not sure if it is a good idea to fix the key-issue as part of
> XWIKI-13623. I think that XWIKI-13623 should only focus on the mentioned
> memory visability and resulting data loss issue.

I did not told you to do it as part of XWIKI-13623 but you cannot do
XWIKI-13623 without cleaning a bit the way cache key is handled right
now in the method since from what I understood you depend on a proper
cache key (to access the right DocumentLoader) in the solution you
proposed.

>
> Fabian.
>
> 2016-08-10 10:52 GMT+02:00 Thomas Mortagne <[hidden email]>:
>
>> On Wed, Aug 10, 2016 at 10:14 AM, Fabian Pichler
>> <[hidden email]> wrote:
>> > Hi Thomas
>> >
>> > Thanks for responding and thanks for supporting the suggested solution.
>> >
>> > I understand the doc.Key-issue that you mentioned. Yet, I think this is
>> not
>> > a
>> > XWikiCacheStore#loadXWikDoc concern. In my understanding the expected
>> > document
>> > to load is defined by the document-reference and the language. Thus the
>> > Hibernate-Store must be concerned in setting the context database
>> according
>> > to the Wiki-Reference before loading the document. This is acctually
>> already
>> > done in the XWiki#getDocument call. I agree that this might be the wrong
>> > place to do it.
>> > Thus, I would suggest to fix this issue in the XWikiHibernateStore on a
>> > seperate jira-issue.
>>
>> In an ideal world it would be fixed in XWikiHibernateStore I agree but
>> this would break retro compatibility (yeah always a pain). That's why
>> I talked about providing a new API.
>>
>> So right now in practice using a combination of context wiki and
>> document reference is part of the API definition and can be applied at
>> XWikiCacheStore too.
>>
>> Note that in practice this is actually a pretty rare case since anyone
>> is supposed to use XWiki#getDocument which make sure the context
>> contain the document wiki. But this can still happen since this is a
>> very old and public API.
>>
>> > Maybe this could be combined with a significant performance improvment in
>> > loading
>> > the documents with fewer sql-queries per loading action.
>> >
>> > With XWIKI-13623 I try to tackle the issue about randomly loosing
>> BaseObject
>> > data because of an not thread safe construction and publication of
>> > XWikiDocument
>> > java objects in XWikiCacheStore. Because this issue gave us a big
>> headache
>> > in
>> > production multiple times, I solved it in our project in the way
>> mentioned.
>> >
>> > If the xwiki-dev team supports the suggested solution, I would be willing
>> > to proceed
>> > and prepare a pull request for an adapted XWikiCacheStore fix.
>> >
>> > Kind regards,
>> > Fabian
>> >
>> >
>> > 2016-08-08 12:07 GMT+02:00 Thomas Mortagne <[hidden email]>:
>> >
>> >> XWikiCacheStore#loadXWikiDoc is actually a bit tricker than that right
>> >> now for retro compatibility and laziness (really need to rewrite this
>> >> whole store API) reasons and before applying what you propose the way
>> >> it deals with document uid should be modified a bit.
>> >>
>> >> Hibernate store is always loading the document from the wiki indicated
>> >> in the context completely ignoring the wiki in the XWikiDocument
>> >> object. That means that doc.getKey() may be different than the key of
>> >> the document that will actually come from the hibernate store. So
>> >> before adding DocumentLoader jingling a first thing to do is generate
>> >> the initial cache key with a mix of the wiki id located in the context
>> >> and the XWikiDocument reference.
>> >>
>> >> Then what you propose sounds good.
>> >>
>> >> On Mon, Aug 8, 2016 at 8:53 AM, Fabian Pichler
>> >> <[hidden email]> wrote:
>> >> > Hi devs
>> >> >
>> >> > http://jira.xwiki.org/browse/XWIKI-13623
>> >> > I would like to discuss the suggested solution, before I start
>> >> implementing
>> >> > it:
>> >> >
>> >> > *I suggest the following solution:*
>> >> > Introducing a nested DocumentLoader class which loads the document
>> >> calling
>> >> > the basestore loadXWikiDoc in a synchronized getDocOrLoad method. The
>> >> > DocumentLoader objects gets instantiated in the cacheStore on a cache
>> >> miss.
>> >> > It then gets stored in a ConcurrentHashMap using the same key which is
>> >> used
>> >> > for the document cache (DocumentReference including language). Using
>> >> > putIfAbsent to insert the DocumentLoader and calling the synchronized
>> >> load
>> >> > method on the object in the map solves the memory visibility issue and
>> >> > moreover ensures that the same XWikiDocument is not loaded multiple
>> times
>> >> > concurrently from the database. It is still possible to load different
>> >> > documents in parallel. A DocumentLoader will load the XWikiDocument
>> only
>> >> > once and the returns the identical document object on any following
>> call
>> >> of
>> >> > the synchronized getDocOrLoad method.
>> >> >
>> >> > WDYT?
>> >> >
>> >> > Kind Regards,
>> >> > Fabian
>> >> > _______________________________________________
>> >> > devs mailing list
>> >> > [hidden email]
>> >> > http://lists.xwiki.org/mailman/listinfo/devs
>> >>
>> >>
>> >>
>> >> --
>> >> Thomas Mortagne
>> >> _______________________________________________
>> >> devs mailing list
>> >> [hidden email]
>> >> http://lists.xwiki.org/mailman/listinfo/devs
>> >>
>> > _______________________________________________
>> > devs mailing list
>> > [hidden email]
>> > http://lists.xwiki.org/mailman/listinfo/devs
>>
>>
>>
>> --
>> Thomas Mortagne
>> _______________________________________________
>> devs mailing list
>> [hidden email]
>> http://lists.xwiki.org/mailman/listinfo/devs
>>
> _______________________________________________
> devs mailing list
> [hidden email]
> http://lists.xwiki.org/mailman/listinfo/devs



--
Thomas Mortagne
_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs
Reply | Threaded
Open this post in threaded view
|

Re: XWikiDocuments not thread safe added to XWikiCacheStore

Fabian Pichler
Thank you Thomas for your explanations.

You are completely right, you wrote it in the first mail. The generation of
the cache key is broken. I added now XWIKI-13632 for this.

If it is ok, I start preparing a PullRequest for XWIKI-13632 and then
a second one for XWIKI-13623.

Kind regards,
Fabian

2016-08-10 16:33 GMT+02:00 Thomas Mortagne <[hidden email]>:

> On Wed, Aug 10, 2016 at 12:41 PM, Fabian Pichler
> <[hidden email]> wrote:
> > ok, if I understand correctly the API contract so far is, that the
> context
> > defines the database used and the reference of the document to load
> > must only be taken relative to the given context. (old String fullName
> > and context-database interpretation).
>
> Yes.
>
> >
> > So, I think it would be better to compute the lookup key based on the
> > context database and the fullName computed out of the document reference.
> > Otherwise you would not get the same document in both situations.
> > 1. from cache
> > 2. freshly loaded
>
> Yes this what I explained.
>
> >
> > Anyway, I am not sure if it is a good idea to fix the key-issue as part
> of
> > XWIKI-13623. I think that XWIKI-13623 should only focus on the mentioned
> > memory visability and resulting data loss issue.
>
> I did not told you to do it as part of XWIKI-13623 but you cannot do
> XWIKI-13623 without cleaning a bit the way cache key is handled right
> now in the method since from what I understood you depend on a proper
> cache key (to access the right DocumentLoader) in the solution you
> proposed.
>
> >
> > Fabian.
> >
> > 2016-08-10 10:52 GMT+02:00 Thomas Mortagne <[hidden email]>:
> >
> >> On Wed, Aug 10, 2016 at 10:14 AM, Fabian Pichler
> >> <[hidden email]> wrote:
> >> > Hi Thomas
> >> >
> >> > Thanks for responding and thanks for supporting the suggested
> solution.
> >> >
> >> > I understand the doc.Key-issue that you mentioned. Yet, I think this
> is
> >> not
> >> > a
> >> > XWikiCacheStore#loadXWikDoc concern. In my understanding the expected
> >> > document
> >> > to load is defined by the document-reference and the language. Thus
> the
> >> > Hibernate-Store must be concerned in setting the context database
> >> according
> >> > to the Wiki-Reference before loading the document. This is acctually
> >> already
> >> > done in the XWiki#getDocument call. I agree that this might be the
> wrong
> >> > place to do it.
> >> > Thus, I would suggest to fix this issue in the XWikiHibernateStore on
> a
> >> > seperate jira-issue.
> >>
> >> In an ideal world it would be fixed in XWikiHibernateStore I agree but
> >> this would break retro compatibility (yeah always a pain). That's why
> >> I talked about providing a new API.
> >>
> >> So right now in practice using a combination of context wiki and
> >> document reference is part of the API definition and can be applied at
> >> XWikiCacheStore too.
> >>
> >> Note that in practice this is actually a pretty rare case since anyone
> >> is supposed to use XWiki#getDocument which make sure the context
> >> contain the document wiki. But this can still happen since this is a
> >> very old and public API.
> >>
> >> > Maybe this could be combined with a significant performance
> improvment in
> >> > loading
> >> > the documents with fewer sql-queries per loading action.
> >> >
> >> > With XWIKI-13623 I try to tackle the issue about randomly loosing
> >> BaseObject
> >> > data because of an not thread safe construction and publication of
> >> > XWikiDocument
> >> > java objects in XWikiCacheStore. Because this issue gave us a big
> >> headache
> >> > in
> >> > production multiple times, I solved it in our project in the way
> >> mentioned.
> >> >
> >> > If the xwiki-dev team supports the suggested solution, I would be
> willing
> >> > to proceed
> >> > and prepare a pull request for an adapted XWikiCacheStore fix.
> >> >
> >> > Kind regards,
> >> > Fabian
> >> >
> >> >
> >> > 2016-08-08 12:07 GMT+02:00 Thomas Mortagne <[hidden email]
> >:
> >> >
> >> >> XWikiCacheStore#loadXWikiDoc is actually a bit tricker than that
> right
> >> >> now for retro compatibility and laziness (really need to rewrite this
> >> >> whole store API) reasons and before applying what you propose the way
> >> >> it deals with document uid should be modified a bit.
> >> >>
> >> >> Hibernate store is always loading the document from the wiki
> indicated
> >> >> in the context completely ignoring the wiki in the XWikiDocument
> >> >> object. That means that doc.getKey() may be different than the key of
> >> >> the document that will actually come from the hibernate store. So
> >> >> before adding DocumentLoader jingling a first thing to do is generate
> >> >> the initial cache key with a mix of the wiki id located in the
> context
> >> >> and the XWikiDocument reference.
> >> >>
> >> >> Then what you propose sounds good.
> >> >>
> >> >> On Mon, Aug 8, 2016 at 8:53 AM, Fabian Pichler
> >> >> <[hidden email]> wrote:
> >> >> > Hi devs
> >> >> >
> >> >> > http://jira.xwiki.org/browse/XWIKI-13623
> >> >> > I would like to discuss the suggested solution, before I start
> >> >> implementing
> >> >> > it:
> >> >> >
> >> >> > *I suggest the following solution:*
> >> >> > Introducing a nested DocumentLoader class which loads the document
> >> >> calling
> >> >> > the basestore loadXWikiDoc in a synchronized getDocOrLoad method.
> The
> >> >> > DocumentLoader objects gets instantiated in the cacheStore on a
> cache
> >> >> miss.
> >> >> > It then gets stored in a ConcurrentHashMap using the same key
> which is
> >> >> used
> >> >> > for the document cache (DocumentReference including language).
> Using
> >> >> > putIfAbsent to insert the DocumentLoader and calling the
> synchronized
> >> >> load
> >> >> > method on the object in the map solves the memory visibility issue
> and
> >> >> > moreover ensures that the same XWikiDocument is not loaded multiple
> >> times
> >> >> > concurrently from the database. It is still possible to load
> different
> >> >> > documents in parallel. A DocumentLoader will load the XWikiDocument
> >> only
> >> >> > once and the returns the identical document object on any following
> >> call
> >> >> of
> >> >> > the synchronized getDocOrLoad method.
> >> >> >
> >> >> > WDYT?
> >> >> >
> >> >> > Kind Regards,
> >> >> > Fabian
> >> >> > _______________________________________________
> >> >> > devs mailing list
> >> >> > [hidden email]
> >> >> > http://lists.xwiki.org/mailman/listinfo/devs
> >> >>
> >> >>
> >> >>
> >> >> --
> >> >> Thomas Mortagne
> >> >> _______________________________________________
> >> >> devs mailing list
> >> >> [hidden email]
> >> >> http://lists.xwiki.org/mailman/listinfo/devs
> >> >>
> >> > _______________________________________________
> >> > devs mailing list
> >> > [hidden email]
> >> > http://lists.xwiki.org/mailman/listinfo/devs
> >>
> >>
> >>
> >> --
> >> Thomas Mortagne
> >> _______________________________________________
> >> devs mailing list
> >> [hidden email]
> >> http://lists.xwiki.org/mailman/listinfo/devs
> >>
> > _______________________________________________
> > devs mailing list
> > [hidden email]
> > http://lists.xwiki.org/mailman/listinfo/devs
>
>
>
> --
> Thomas Mortagne
> _______________________________________________
> devs mailing list
> [hidden email]
> http://lists.xwiki.org/mailman/listinfo/devs
>
_______________________________________________
devs mailing list
[hidden email]
http://lists.xwiki.org/mailman/listinfo/devs
Reply | Threaded
Open this post in threaded view
|

Re: XWikiDocuments not thread safe added to XWikiCacheStore

Thomas Mortagne
Administrator
Great thanks :)

On Wed, Aug 17, 2016 at 10:10 AM, Fabian Pichler
<[hidden email]> wrote:

> Thank you Thomas for your explanations.
>
> You are completely right, you wrote it in the first mail. The generation of
> the cache key is broken. I added now XWIKI-13632 for this.
>
> If it is ok, I start preparing a PullRequest for XWIKI-13632 and then
> a second one for XWIKI-13623.
>
> Kind regards,
> Fabian
>
> 2016-08-10 16:33 GMT+02:00 Thomas Mortagne <[hidden email]>:
>
>> On Wed, Aug 10, 2016 at 12:41 PM, Fabian Pichler
>> <[hidden email]> wrote:
>> > ok, if I understand correctly the API contract so far is, that the
>> context
>> > defines the database used and the reference of the document to load
>> > must only be taken relative to the given context. (old String fullName
>> > and context-database interpretation).
>>
>> Yes.
>>
>> >
>> > So, I think it would be better to compute the lookup key based on the
>> > context database and the fullName computed out of the document reference.
>> > Otherwise you would not get the same document in both situations.
>> > 1. from cache
>> > 2. freshly loaded
>>
>> Yes this what I explained.
>>
>> >
>> > Anyway, I am not sure if it is a good idea to fix the key-issue as part
>> of
>> > XWIKI-13623. I think that XWIKI-13623 should only focus on the mentioned
>> > memory visability and resulting data loss issue.
>>
>> I did not told you to do it as part of XWIKI-13623 but you cannot do
>> XWIKI-13623 without cleaning a bit the way cache key is handled right
>> now in the method since from what I understood you depend on a proper
>> cache key (to access the right DocumentLoader) in the solution you
>> proposed.
>>
>> >
>> > Fabian.
>> >
>> > 2016-08-10 10:52 GMT+02:00 Thomas Mortagne <[hidden email]>:
>> >
>> >> On Wed, Aug 10, 2016 at 10:14 AM, Fabian Pichler
>> >> <[hidden email]> wrote:
>> >> > Hi Thomas
>> >> >
>> >> > Thanks for responding and thanks for supporting the suggested
>> solution.
>> >> >
>> >> > I understand the doc.Key-issue that you mentioned. Yet, I think this
>> is
>> >> not
>> >> > a
>> >> > XWikiCacheStore#loadXWikDoc concern. In my understanding the expected
>> >> > document
>> >> > to load is defined by the document-reference and the language. Thus
>> the
>> >> > Hibernate-Store must be concerned in setting the context database
>> >> according
>> >> > to the Wiki-Reference before loading the document. This is acctually
>> >> already
>> >> > done in the XWiki#getDocument call. I agree that this might be the
>> wrong
>> >> > place to do it.
>> >> > Thus, I would suggest to fix this issue in the XWikiHibernateStore on
>> a
>> >> > seperate jira-issue.
>> >>
>> >> In an ideal world it would be fixed in XWikiHibernateStore I agree but
>> >> this would break retro compatibility (yeah always a pain). That's why
>> >> I talked about providing a new API.
>> >>
>> >> So right now in practice using a combination of context wiki and
>> >> document reference is part of the API definition and can be applied at
>> >> XWikiCacheStore too.
>> >>
>> >> Note that in practice this is actually a pretty rare case since anyone
>> >> is supposed to use XWiki#getDocument which make sure the context
>> >> contain the document wiki. But this can still happen since this is a
>> >> very old and public API.
>> >>
>> >> > Maybe this could be combined with a significant performance
>> improvment in
>> >> > loading
>> >> > the documents with fewer sql-queries per loading action.
>> >> >
>> >> > With XWIKI-13623 I try to tackle the issue about randomly loosing
>> >> BaseObject
>> >> > data because of an not thread safe construction and publication of
>> >> > XWikiDocument
>> >> > java objects in XWikiCacheStore. Because this issue gave us a big
>> >> headache
>> >> > in
>> >> > production multiple times, I solved it in our project in the way
>> >> mentioned.
>> >> >
>> >> > If the xwiki-dev team supports the suggested solution, I would be
>> willing
>> >> > to proceed
>> >> > and prepare a pull request for an adapted XWikiCacheStore fix.
>> >> >
>> >> > Kind regards,
>> >> > Fabian
>> >> >
>> >> >
>> >> > 2016-08-08 12:07 GMT+02:00 Thomas Mortagne <[hidden email]
>> >:
>> >> >
>> >> >> XWikiCacheStore#loadXWikiDoc is actually a bit tricker than that
>> right
>> >> >> now for retro compatibility and laziness (really need to rewrite this
>> >> >> whole store API) reasons and before applying what you propose the way
>> >> >> it deals with document uid should be modified a bit.
>> >> >>
>> >> >> Hibernate store is always loading the document from the wiki
>> indicated
>> >> >> in the context completely ignoring the wiki in the XWikiDocument
>> >> >> object. That means that doc.getKey() may be different than the key of
>> >> >> the document that will actually come from the hibernate store. So
>> >> >> before adding DocumentLoader jingling a first thing to do is generate
>> >> >> the initial cache key with a mix of the wiki id located in the
>> context
>> >> >> and the XWikiDocument reference.
>> >> >>
>> >> >> Then what you propose sounds good.
>> >> >>
>> >> >> On Mon, Aug 8, 2016 at 8:53 AM, Fabian Pichler
>> >> >> <[hidden email]> wrote:
>> >> >> > Hi devs
>> >> >> >
>> >> >> > http://jira.xwiki.org/browse/XWIKI-13623
>> >> >> > I would like to discuss the suggested solution, before I start
>> >> >> implementing
>> >> >> > it:
>> >> >> >
>> >> >> > *I suggest the following solution:*
>> >> >> > Introducing a nested DocumentLoader class which loads the document
>> >> >> calling
>> >> >> > the basestore loadXWikiDoc in a synchronized getDocOrLoad method.
>> The
>> >> >> > DocumentLoader objects gets instantiated in the cacheStore on a
>> cache
>> >> >> miss.
>> >> >> > It then gets stored in a ConcurrentHashMap using the same key
>> which is
>> >> >> used
>> >> >> > for the document cache (DocumentReference including language).
>> Using
>> >> >> > putIfAbsent to insert the DocumentLoader and calling the
>> synchronized
>> >> >> load
>> >> >> > method on the object in the map solves the memory visibility issue
>> and
>> >> >> > moreover ensures that the same XWikiDocument is not loaded multiple
>> >> times
>> >> >> > concurrently from the database. It is still possible to load
>> different
>> >> >> > documents in parallel. A DocumentLoader will load the XWikiDocument
>> >> only
>> >> >> > once and the returns the identical document object on any following
>> >> call
>> >> >> of
>> >> >> > the synchronized getDocOrLoad method.
>> >> >> >
>> >> >> > WDYT?
>> >> >> >
>> >> >> > Kind Regards,
>> >> >> > Fabian
>> >> >> > _______________________________________________
>> >> >> > devs mailing list
>> >> >> > [hidden email]
>> >> >> > http://lists.xwiki.org/mailman/listinfo/devs
>> >> >>
>> >> >>
>> >> >>
>> >> >> --
>> >> >> Thomas Mortagne
>> >> >> _______________________________________________
>> >> >> devs mailing list
>> >> >> [hidden email]
>> >> >> http://lists.xwiki.org/mailman/listinfo/devs
>> >> >>
>> >> > _______________________________________________
>> >> > devs mailing list
>> >> > [hidden email]
>> >> > http://lists.xwiki.org/mailman/listinfo/devs
>> >>
>> >>
>> >>
>> >> --
>> >> Thomas Mortagne
>> >> _______________________________________________
>> >> devs mailing list
>> >> [hidden email]
>> >> http://lists.xwiki.org/mailman/listinfo/devs
>> >>
>> > _______________________________________________
>> > devs mailing list
>> > [hidden email]
>> > http://lists.xwiki.org/mailman/listinfo/devs
>>
>>
>>
>> --
>> Thomas Mortagne
>> _______________________________________________
>> devs mailing list
>> [hidden email]
>> http://lists.xwiki.org/mailman/listinfo/devs
>>
> _______________________________________________
> devs mailing list
> [hidden email]
> http://lists.xwiki.org/mailman/listinfo/devs



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