Re: [xwiki-devs] [xwiki-notifications] r24158 - in platform/web/trunk: gwt/src/main/java/com/xpn/xwiki/gwt/api/server wysiwyg/src/main/java/com/xpn/xwiki/wysiwyg/server
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 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. Can you please explain? 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
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
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
participants (2)
-
Anca Paula Luca -
Vincent Massol