On 10/01/2009 12:16 PM, Anca Paula Luca wrote:
> Hi Vincent,
>
> On 10/01/2009 11:18 AM, Vincent Massol wrote:
>> Hi Anca,
>>
>> Be very very careful with ThreadLocal. It's very easy to make mistakes
>> and cause memory leaks. Actually I think you may have introduced one
>> already ;) (please do some web research on ThreadLocal you'll see it
>> has some caveats and they must absolutely be freed when the thread
>> exits).
>
> I wasn't aware of that (I just found extensive complains and references in a
> lucene issue). However, I could ensure that the threadlocal is removed after
> the
> call is done.
>
>>
>> I believe this needs to be discussed more to ensure it's the right way
>> and doesn't cause pbs.
>>
>> Also we already have an ExecutionContext so I'm not sure why we need
>> this.
>
> indeed we don't. Seems that all initialization (execution context et all,
> XWikiContext set on execution context) is done in:
>
> XWikiServiceImpl.initializeContainerComponent
>
> and here, in the service, we should just do:
>
> protected XWikiContext getXWikiContext() {
> return (XWikiContext)
> Utils.getComponent(Execution.class).getContext().getProperty("xwikicontext");
> }
fyi, commited this fix in r24172. I tested this on local and the functions that
use the context don't seem to be affected. I cannot test the thread safety
though.
Happy coding,
Anca
>
>>
>> Can you please explain?
>
> I wrongly remembered we have a good reason to not use the execution context
> mechanism to get context data, such as "not initialized in the GWT Service",
> but
> it seems it's not the case.
>
> In an ideal threadlocal impl, this solution shouldn't have been buggy, just
> suboptimal.
>
> Thanks a lot,
> Anca
>
>>
>> Thanks
>> -Vincent
>>
>>
>> On Oct 1, 2009, at 10:13 AM, lucaa (SVN) wrote:
>>
>>> Author: lucaa
>>> Date: 2009-10-01 10:13:20 +0200 (Thu, 01 Oct 2009)
>>> New Revision: 24158
>>>
>>> Modified:
>>> platform/web/trunk/gwt/src/main/java/com/xpn/xwiki/gwt/api/server/
>>> XWikiServiceImpl.java
>>> platform/web/trunk/wysiwyg/src/main/java/com/xpn/xwiki/wysiwyg/
>>> server/DefaultWysiwygService.java
>>> Log:
>>> XWIKI-4411: XWikiServiceImpl's xwiki context is not handled threadsafe
>>> * handled the prepared context in a ThreadLocal member.
>>>
>>>
>>> Modified: platform/web/trunk/gwt/src/main/java/com/xpn/xwiki/gwt/api/
>>> server/XWikiServiceImpl.java
>>> ===================================================================
>>> --- platform/web/trunk/gwt/src/main/java/com/xpn/xwiki/gwt/api/
>>> server/XWikiServiceImpl.java 2009-09-30 21:49:09 UTC (rev 24157)
>>> +++ platform/web/trunk/gwt/src/main/java/com/xpn/xwiki/gwt/api/
>>> server/XWikiServiceImpl.java 2009-10-01 08:13:20 UTC (rev 24158)
>>> @@ -66,7 +66,7 @@
>>> {
>>> private static final Log LOG = LogFactory.getLog(XWiki.class);
>>>
>>> - private XWikiContext context;
>>> + private ThreadLocal<XWikiContext> context;
>>>
>>> /**
>>> * We override the default processCall method in order to
>>> provide XWiki initialization before
>>> @@ -131,7 +131,7 @@
>>> }
>>>
>>> context.put("ajax", new Boolean(true));
>>> - this.context = context;
>>> + this.context.set(context);
>>> }
>>>
>>> private void initializeContainerComponent(XWikiContext context)
>>> @@ -168,7 +168,7 @@
>>>
>>> protected XWikiContext getXWikiContext()
>>> {
>>> - return this.context;
>>> + return this.context.get();
>>> }
>>>
>>> protected XWikiGWTException getXWikiGWTException(Exception e) {
>>>
>>> Modified: platform/web/trunk/wysiwyg/src/main/java/com/xpn/xwiki/
>>> wysiwyg/server/DefaultWysiwygService.java
>>> ===================================================================
>>> --- platform/web/trunk/wysiwyg/src/main/java/com/xpn/xwiki/wysiwyg/
>>> server/DefaultWysiwygService.java 2009-09-30 21:49:09 UTC (rev 24157)
>>> +++ platform/web/trunk/wysiwyg/src/main/java/com/xpn/xwiki/wysiwyg/
>>> server/DefaultWysiwygService.java 2009-10-01 08:13:20 UTC (rev 24158)
>>> @@ -302,14 +302,8 @@
>>> getXWikiContext().setDatabase(wikiName);
>>> }
>>> spaceNamesList =
>>> getXWikiContext().getWiki().getSpaces(getXWikiContext());
>>> - // get the blacklisted spaces from the session as
>>> they've been set in xwikivars.vm, when the page edited
>>> - // with this wysiwyg was loaded
>>> - // TODO: remove this when the public API will exclude
>>> them by default, or they'll be set in the config
>>> - List<String> blacklistedSpaces =
>>> - (ArrayList<String>)
>>> getThreadLocalRequest
>>> ().getSession().getAttribute("blacklistedSpaces");
>>> - if (blacklistedSpaces != null&&
>>> blacklistedSpaces.size()> 0) {
>>> - spaceNamesList.removeAll(blacklistedSpaces);
>>> - }
>>> + // remove the blacklisted spaces from the all spaces list
>>> + spaceNamesList.removeAll(getBlackListedSpaces());
>>> Collections.sort(spaceNamesList);
>>> } catch (XWikiException e) {
>>> e.printStackTrace();
>>> @@ -322,6 +316,26 @@
>>> }
>>>
>>> /**
>>> + * Helper function to retrieve the blacklisted spaces in this
>>> session, as they've been set in xwikivars.vm, when the
>>> + * page edited with this wysiwyg was loaded.<br />
>>> + * TODO: remove this when the public API will exclude them by
>>> default, or they'll be set in the config.
>>> + *
>>> + * @return the list of blacklisted spaces from the session
>>> + */
>>> + @SuppressWarnings("unchecked")
>>> + private List<String> getBlackListedSpaces()
>>> + {
>>> + // get the blacklisted spaces from the session
>>> + List<String> blacklistedSpaces =
>>> + (ArrayList<String>)
>>> getThreadLocalRequest
>>> ().getSession().getAttribute("blacklistedSpaces");
>>> + // always return a list, even if blacklisted spaces
>>> variable wasn't set
>>> + if (blacklistedSpaces == null) {
>>> + blacklistedSpaces = new ArrayList<String>();
>>> + }
>>> + return blacklistedSpaces;
>>> + }
>>> +
>>> + /**
>>> * {...@inheritdoc}
>>> *
>>> * @see WysiwygService#getPageNames(String, String)
>>> @@ -379,12 +393,31 @@
>>> throws XWikiGWTException
>>> {
>>> try {
>>> + String quote = "'";
>>> + String doubleQuote = "''";
>>> // FIXME: this fullname comparison with the keyword does
>>> not contain the wiki name
>>> - String escapedKeyword = keyword.replaceAll("'",
>>> "''").toLowerCase();
>>> + String escapedKeyword = keyword.replaceAll(quote,
>>> doubleQuote).toLowerCase();
>>> + // add condition for the doc to not be in the list of
>>> blacklisted spaces.
>>> + // TODO: might be a pb with scalability of this
>>> + String noBlacklistedSpaces = "";
>>> + List<String> blackListedSpaces = getBlackListedSpaces();
>>> + if (!blackListedSpaces.isEmpty()) {
>>> + StringBuffer spacesList = new StringBuffer();
>>> + for (String bSpace : blackListedSpaces) {
>>> + if (spacesList.length()> 0) {
>>> + spacesList.append(", ");
>>> + }
>>> + spacesList.append(quote);
>>> + spacesList.append(bSpace.replaceAll(quote,
>>> doubleQuote));
>>> + spacesList.append(quote);
>>> + }
>>> + noBlacklistedSpaces = "doc.web not in (" +
>>> spacesList.toString() + ")";
>>> + }
>>> List<XWikiDocument> docs =
>>> getXWikiContext().getWiki().search(
>>> - "select distinct doc from XWikiDocument as doc
>>> where lower(doc.title) like '%" + escapedKeyword
>>> - + "%' or lower(doc.fullName) like '%" +
>>> escapedKeyword + "%'", count, start, getXWikiContext());
>>> + "select distinct doc from XWikiDocument as doc
>>> where " + noBlacklistedSpaces
>>> + + " and (lower(doc.title) like '%" +
>>> escapedKeyword + "%' or lower(doc.fullName) like '%"
>>> + + escapedKeyword + "%')", count, start,
>>> getXWikiContext());
>>> return prepareDocumentResultsList(docs);
>>> } catch (XWikiException e) {
>>> throw getXWikiGWTException(e);
>>> @@ -406,7 +439,7 @@
>>> for (XWikiDocument doc : docs) {
>>> com.xpn.xwiki.gwt.api.client.Document xwikiDoc = new
>>> com.xpn.xwiki.gwt.api.client.Document();
>>> xwikiDoc.setFullName(doc.getFullName());
>>> -
>>> xwikiDoc.setTitle(doc.getDisplayTitle(getXWikiContext()));
>>> +
>>> xwikiDoc.setTitle(doc.getRenderedTitle(Syntax.XHTML_1_0,
>>> getXWikiContext()));
>>> // FIXME: shouldn't use upload URL here, but since we
>>> don't want to add a new field...
>>> xwikiDoc.setUploadURL(doc.getURL(VIEW_ACTION,
>>> getXWikiContext()));
>>> results.add(xwikiDoc);
>>>
>>> _______________________________________________
>>> notifications mailing list
>>> [email protected]
>>> http://lists.xwiki.org/mailman/listinfo/notifications
>>
>> _______________________________________________
>> devs mailing list
>> [email protected]
>> http://lists.xwiki.org/mailman/listinfo/devs
> _______________________________________________
> devs mailing list
> [email protected]
> http://lists.xwiki.org/mailman/listinfo/devs
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs
