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");
}
>
> 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
