You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by "Josh Elser (JIRA)" <ji...@apache.org> on 2018/08/21 16:13:00 UTC
[jira] [Commented] (PHOENIX-4860) CursorUtil Needs to Use a
ConcurrentHashMap rather than HashMap
[ https://issues.apache.org/jira/browse/PHOENIX-4860?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16587663#comment-16587663 ]
Josh Elser commented on PHOENIX-4860:
-------------------------------------
Thanks for the patch, Jack!
I think your patch would help as-is, but there are still some lurking concurrency problems with how the methods are written. Using a concurrent hash map gives us some more flexibility to work around those. For example..
{code:java}
public static boolean declareCursor(DeclareCursorStatement stmt, QueryPlan queryPlan) throws SQLException {
if(mapCursorIDQuery.containsKey(stmt.getCursorName())){
throw new SQLException("Can't declare cursor " + stmt.getCursorName() + ", cursor identifier already in use.");
} else {
mapCursorIDQuery.put(stmt.getCursorName(), new CursorWrapper(stmt.getCursorName(), stmt.getQuerySQL(), queryPlan));
return true;
}
}{code}
Could be changed to be:
{code:java}
public static boolean declareCursor(DeclareCursorStatement stmt, QueryPlan queryPlan) throws SQLException {
CursorWrapper newWrapper = new CursorWrapper(stmt.getCursorName(), stmt.getQuerySQL(), queryPlan);
CursorWrapper previous = mapCursorIDQuery.putIfAbsent(stmt.getCursorName(), newWrapper);
// The ConcurrentHashMap returns null when there was no previous value associated with this key
if (previous != null) {
throw new SQLException("Can't declare cursor " + stmt.getCursorName() + ", cursor identifier already in use.");
}
return true;
}{code}
This would avoid the race condition where someone modifies the map inbetween our {{containsKey}} and {{put}} calls on the Map.
Does that make sense, and would that be something you're interesting in also fixing?
FYI [~ankit@apache.org], [~gsbiju]
> CursorUtil Needs to Use a ConcurrentHashMap rather than HashMap
> ---------------------------------------------------------------
>
> Key: PHOENIX-4860
> URL: https://issues.apache.org/jira/browse/PHOENIX-4860
> Project: Phoenix
> Issue Type: Bug
> Affects Versions: 4.14.0, 4.13.1, 5.0.0
> Reporter: Jack Steenkamp
> Priority: Major
> Attachments: CursorUtil.patch
>
>
> in very rare cases, when dealing with Apache Phoenix Cursors, the following NullPointerException is encountered:
> java.lang.NullPointerException: null
> at org.apache.phoenix.util.CursorUtil.updateCursor(CursorUtil.java:179)
> at org.apache.phoenix.iterate.CursorResultIterator.next(CursorResultIterator.java:46)
> at org.apache.phoenix.jdbc.PhoenixResultSet.next(PhoenixResultSet.java:779)
> (This is for 4.13.1 - but seems that
> org.apache.phoenix.util.CursorUtil has not changed, at the time of writing, since first being introduced as part of PHOENIX-3572).
> Upon closer inspection it would seem that on line 124 of CursorUtil, a
> HashMap is used to keep state which is then exposed via a number of
> static methods, which, one has to assume, can be accessed by many
> different threads. Using a plain old HashMap in cases like these can cause issues.
> The mapCursorIDQuery member should be a ConcurrentHashMap instead? That should tighten up the class and prevent any potential inconsistencies.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)