• Bug
  • Status: Closed
  • 2 Major
  • Resolution: Won't Fix
  • cdennis
  • Reporter: teck
  • February 16, 2010
  • 0
  • Watchers: 1
  • March 24, 2011
  • February 16, 2011

Description

The change made in rev 20575 for /tim-concurrent-collections/src/main/java/org/terracotta/collections/ConcurrentDistributedMapDso.java has a slight issue in it.

When the exception handling is called from the path of unlockedPut(), it is possible that the value restored in the store isn’t quite the correct one (in the face of concurrent puts on the same key).

The following is the email thread documenting all known discussion on the matter:

—–Original Message—– From: Chris Dennis [mailto:[email protected]] Sent: Friday, February 12, 2010 6:20 AM To: Tim Eck Cc: ‘Saravanan Subbiah’; [email protected] Subject: Re: [Transparency] review?

Other than this, this looks okay… For the problem that Saro highlighted you should probably be using the replaceUsingReferenceEquality (and maybe adding a reference equality variant of the two-arg remove as well).

On Feb 11, 2010, at 7:38 PM, Tim Eck wrote:

Good point. I assume meant that I should always be using replace() and the 2-arg remove() for all cases, not just for the exception handling from the call path of unlockedPut(), right?

I also need to see if this fail to restore the mapping if something is flushing values at the same time.

Suppose it would be better to not mutate locally until we know we won’t throw an exception instead of trying to clean up after the fact. That change is harder to write :-(

—–Original Message—– From: Saravanan Subbiah [mailto:[email protected]] Sent: Thursday, February 11, 2010 4:21 PM To: Tim Eck Cc: [email protected] Subject: Re: review?

Just one minor comment.

In unlockedPut(), since its not protected by a key-level lock, on an exception, you may want to do this instead.

  • } catch (TCNonPortableObjectError npoe) {
  • if (old != null) {
  • store.replace(key, value, old);
  • } else {
  • store.remove(key, value);
  • }
  • throw npoe;
  • }

Comments

Fiona OShea 2010-02-23

where did we leave this? fix or leave as is?

Tim Eck 2010-02-23

I think this should still be cleaned up. unlockedPut() is still kinda wrong and in general this handling should be completely redone such that we never even make the local mutation in the face of non-portable errors (rather fix it after the fact like the code does now).

Fiona OShea 2010-11-02

Does this issue affect express in any way?

Tim Eck 2010-11-02

I don’t think this affects express since we shouldn’t be dealing with non-portable types ever in that context.

Chris Dennis 2011-02-11

This is a sub-task of DEV-5391, but cannot be made one since it is in a separate project.

Chris Dennis 2011-02-16

CDM is going to be deprecated so this will not be fixed.