You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by "Robert Stupp (JIRA)" <ji...@apache.org> on 2018/12/10 16:00:00 UTC
[jira] [Comment Edited] (CASSANDRA-14871) Severe concurrency issues
in STCS,DTCS,TWCS,TMD.Topology,TypeParser
[ https://issues.apache.org/jira/browse/CASSANDRA-14871?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16714681#comment-16714681 ]
Robert Stupp edited comment on CASSANDRA-14871 at 12/10/18 3:59 PM:
--------------------------------------------------------------------
I'm fine with using {{Verify}}, but it should include the string that yielded {{null}}. Like {{Verify.verify(type != null, "Parsing %s yielded null, which is a bug", str)}} and move that after the assignment of {{type}}. That way we don't need the {{cache.get(str)}} in the synchronized block, because {{type}} is then guaranteed to be non-null.
EDIT: So instead of
{code:java}
synchronized (TypeParser.class)
{
if (!cache.containsKey(str))
{
ImmutableMap.Builder<String, AbstractType<?>> builder = ImmutableMap.builder();
builder.putAll(cache).put(str, type);
cache = builder.build();
}
AbstractType<?> rtype = cache.get(str);
Verify.verify(rtype != null);
return rtype;
}
{code}
like this:
{code:java}
if (!isEOS(str, i) && str.charAt(i) == '(')
type = getAbstractType(name, new TypeParser(str, i));
else
type = getAbstractType(name);
Verify.verify(type != null, "Parsing %s yielded null, which is a bug", str);
// <comments>
synchronized (TypeParser.class)
{
if (!cache.containsKey(str))
{
ImmutableMap.Builder<String, AbstractType<?>> builder = ImmutableMap.builder();
builder.putAll(cache).put(str, type);
cache = builder.build();
}
return type;
}
{code}
was (Author: snazy):
I'm fine with using {{Verify}}, but it should include the string that yielded {{null}}. Like {{Verify.verify(type != null, "Parsing %s yielded null, which is a bug", type)}} and move that after the assignment of {{type}}. That way we don't need the {{cache.get(str)}} in the synchronized block, because {{type}} is then guaranteed to be non-null.
{code:java}
synchronized (TypeParser.class)
{
if (!cache.containsKey(str))
{
ImmutableMap.Builder<String, AbstractType<?>> builder = ImmutableMap.builder();
builder.putAll(cache).put(str, type);
cache = builder.build();
}
AbstractType<?> rtype = cache.get(str);
Verify.verify(rtype != null);
return rtype;
}
{code}
> Severe concurrency issues in STCS,DTCS,TWCS,TMD.Topology,TypeParser
> -------------------------------------------------------------------
>
> Key: CASSANDRA-14871
> URL: https://issues.apache.org/jira/browse/CASSANDRA-14871
> Project: Cassandra
> Issue Type: Bug
> Components: Core
> Reporter: Robert Stupp
> Assignee: Robert Stupp
> Priority: Critical
> Fix For: 4.0, 3.0.x, 3.11.x
>
>
> There are a couple of places in the code base that do not respect that j.u.HashMap + related classes are not thread safe and some parts rely on internals of the implementation of HM, which can change.
> We have observed failures like {{NullPointerException}} and {{ConcurrentModificationException}} as well as wrong behavior.
> Affected areas in the code base:
> * {{SizeTieredCompactionStrategy}}
> * {{DateTieredCompactionStrategy}}
> * {{TimeWindowCompactionStrategy}}
> * {{TokenMetadata.Topology}}
> * {{TypeParser}}
> * streaming / concurrent access to {{LifecycleTransaction}} (handled in CASSANDRA-14554)
> While the patches for the compaction strategies + {{TypeParser}} are pretty straight forward, the patch for {{TokenMetadata.Topology}} requires it to be made immutable.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org