New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SI-7943 -- make `TrieMap.getOrElseUpdate` atomic. #4319

Merged
merged 1 commit into from Feb 19, 2015

Conversation

Projects
None yet
7 participants
@axel22
Member

axel22 commented Feb 11, 2015

Override getOrElseUpdate method in TrieMap.
The signature and contract of this method corresponds closely to the
computeIfAbsent from java.util.concurrent.ConcurrentMap.
Eventually, computeIfAbsent should be added to
scala.collection.concurrent.Map.

Add tests.

Review by @Ichoran

@scala-jenkins scala-jenkins added this to the 2.11.6 milestone Feb 11, 2015

@axel22

This comment has been minimized.

Member

axel22 commented Feb 11, 2015

This is what I get with the vanilla TrieMap in Scala 2.11:

scala> Test.main(Array())

  • concurrent.TrieMap.concurrent growing snapshots: OK, passed 100 tests.
  • concurrent.TrieMap.update: OK, passed 100 tests.
  • concurrent.TrieMap.concurrent update: OK, passed 100 tests.
  • concurrent.TrieMap.concurrent remove: OK, passed 100 tests.
  • concurrent.TrieMap.concurrent putIfAbsent: OK, passed 100 tests.
    ! concurrent.TrieMap.concurrent getOrElseUpdate: Falsified after 0 passed t
    ests.

    ARG_0: 2
    ARG_0_ORIGINAL: 15
    ARG_1: 38
    ARG_1_ORIGINAL: 42260
    java.lang.RuntimeException: Nonzero exit code: 1
    at scala.sys.package$.error(package.scala:27)
    at sbt.Defaults$$anonfun$consoleTask$1$$anonfun$apply$38.apply(Defaults.scala:766)
    at sbt.Defaults$$anonfun$consoleTask$1$$anonfun$apply$38.apply(Defaults.scala:766)
    at scala.Option.foreach(Option.scala:236)
    at sbt.Defaults$$anonfun$consoleTask$1.apply(Defaults.scala:766)
    at sbt.Defaults$$anonfun$consoleTask$1.apply(Defaults.scala:761)
    at scala.Function8$$anonfun$tupled$1.apply(Function8.scala:35)
    at scala.Function8$$anonfun$tupled$1.apply(Function8.scala:34)
    at scala.Function1$$anonfun$compose$1.apply(Function1.scala:47)
    at sbt.$tilde$greater$$anonfun$$u2219$1.apply(TypeFunctions.scala:40)
    at sbt.std.Transform$$anon$4.work(System.scala:63)
    at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:226)
    at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:226)
    at sbt.ErrorHandling$.wideConvert(ErrorHandling.scala:17)
    at sbt.Execute.work(Execute.scala:235)
    at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:226)
    at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:226)
    at sbt.ConcurrentRestrictions$$anon$4$$anonfun$1.apply(ConcurrentRestrictions.scala:159)
    at sbt.CompletionService$$anon$2.call(CompletionService.scala:28)
    at java.util.concurrent.FutureTask.run(FutureTask.java:262)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:471)
    at java.util.concurrent.FutureTask.run(FutureTask.java:262)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
    at java.lang.Thread.run(Thread.java:745)

@axel22 axel22 force-pushed the axel22:fix-si-7943 branch from 2bed993 to 9bd26ad Feb 11, 2015

@axel22

This comment has been minimized.

Member

axel22 commented Feb 11, 2015

Updated exception message.

@axel22 axel22 force-pushed the axel22:fix-si-7943 branch from 9bd26ad to 32404e6 Feb 11, 2015

* Note: This method will invoke op at most once.
* However, `op` may be invoked without the result being added to the map if
* a concurrent process is also trying to add a value corresponding to the
* same key k.

This comment has been minimized.

@Ichoran

Ichoran Feb 18, 2015

Contributor

Very nitpicky, but k should be in backquotes.

@@ -178,6 +178,10 @@ trait MapLike[A, B, +This <: MapLike[A, B, This] with Map[A, B]]
*
* Otherwise, computes value from given expression `op`, stores with key
* in map and returns that value.
*
* Concurrent map implementations may evaluate the expression `op`
* multiple times.

This comment has been minimized.

@Ichoran

Ichoran Feb 18, 2015

Contributor

Do we want to add, "or may evaluate op without inserting the result"?

@Ichoran

This comment has been minimized.

Contributor

Ichoran commented Feb 18, 2015

LGTM but could perhaps be a bit better with tiny doc changes.

Fix SI-7943 -- make `TrieMap.getOrElseUpdate` atomic.
Override `getOrElseUpdate` method in `TrieMap`.
The signature and contract of this method corresponds closely to the
`computeIfAbsent` from `java.util.concurrent.ConcurrentMap`.
Eventually, `computeIfAbsent` should be added to
`scala.collection.concurrent.Map`.

Add tests.

Review by @Ichoran

@axel22 axel22 force-pushed the axel22:fix-si-7943 branch from 32404e6 to da2d4ca Feb 18, 2015

@axel22

This comment has been minimized.

Member

axel22 commented Feb 18, 2015

Docs fixed.

@Ichoran

This comment has been minimized.

Contributor

Ichoran commented Feb 18, 2015

LGTM without qualifications!

@adriaanm

This comment has been minimized.

Member

adriaanm commented Feb 18, 2015

Thanks, @axel22! Good to see you pop up here so regularly :-) 👋

@axel22

This comment has been minimized.

Member

axel22 commented Feb 18, 2015

Glad to be of help! :)

adriaanm added a commit that referenced this pull request Feb 19, 2015

Merge pull request #4319 from axel22/fix-si-7943
Fix SI-7943 -- make `TrieMap.getOrElseUpdate` atomic.

@adriaanm adriaanm merged commit 812790a into scala:2.11.x Feb 19, 2015

4 checks passed

integrate-ide [303] SUCCESS. Took 60 min.
Details
validate-main [532] SUCCESS. Took 158 min.
Details
validate-publish-core [536] SUCCESS. Took 15 min.
Details
validate-test [268] SUCCESS. Took 99 min.
Details
@2rs2ts

This comment has been minimized.

2rs2ts commented Apr 1, 2015

Not sure if this is the same as computeIfAbsent. It's more like putIfAbsent but with a lazy argument. I realize that this PR made it thread-safe but it doesn't correspond to computeIfAbsent as described in the PR description.

@axel22

This comment has been minimized.

Member

axel22 commented Apr 1, 2015

Description says "closely corresponds", not "the same as". Also, a caller of computeIfAbsent has the key, so can do effectively the same as if the second argument were of type K => V, assuming that all the useful information in a key is defined by equality.

@2rs2ts

This comment has been minimized.

2rs2ts commented Apr 1, 2015

True, it can be worked around. This just threw us for a loop at work because we were still getting exceptions.

@axel22

This comment has been minimized.

Member

axel22 commented Apr 1, 2015

Can you describe say a bit more about what the exception was? Was it with this change, or with an earlier Scala release?

@2rs2ts

This comment has been minimized.

2rs2ts commented Apr 1, 2015

It was with this change. We had code which should only be called once and would throw an exception if it was called twice. Its result was to be stored in the TrieMap, like ourMap.getOrElseUpdate(key, codeWhichShouldOnlyBeCalledOnce()). We found that if we were quick enough we could reliably make the exception occur.

@axel22

This comment has been minimized.

Member

axel22 commented Apr 1, 2015

Was the getOrElseUpdate operation being invoked by multiple threads?

@2rs2ts

This comment has been minimized.

2rs2ts commented Apr 1, 2015

Yes, indeed it was. You provided this caveat in the documentation, we just failed to heed it because we assumed it would work like computeIfAbsent.

@axel22

This comment has been minimized.

Member

axel22 commented Apr 1, 2015

Well, computeIfAbsent technically works that way only for ConcurrentHashMaps, but not for general ConcurrentMaps.

I'm thinking now whether we should have the execute-exactly-once semantics on TrieMaps.

@Ichoran

This comment has been minimized.

Contributor

Ichoran commented Apr 1, 2015

You don't want to perform the computation once; you want to perform it zero times if you don't end up adding it. But you then have to either literally lock or have a virtual CAS-based lock to reserve time to perform the computation. If it's okay to block for arbitrarily long on a single element waiting for a computation to complete, then it's okay. Overall, I'd say it's better the present way. Maybe the docs need to be even clearer that if you call the method multiple times, you may get one execution of op per call if the map is in the process of having that key added. (It's a natural consequence of what the docs already say, but thinking about properties when simultaneous updates are attempted is not so easy; if they were, concurrent programming wouldn't be so hard.)

@axel22

This comment has been minimized.

Member

axel22 commented Apr 2, 2015

Essentially, a lock on a lazily-evaluated node would be required. We had a long discussion about this: https://issues.scala-lang.org/browse/SI-7943

I tend to fall in your camp about it being better the present way, and having the docs improved.

@2rs2ts

This comment has been minimized.

2rs2ts commented Apr 2, 2015

I'd be happy to have the docs improved too. I might open a PR and do just that.

For what it's worth, I found an old implementation of an AtomicMap that boundary made, but it was compiled for 2.9.1. I've brought it up to date, and although I fear it's abandonware, I did make a PR to their repo for my changes. So for those interested in something that ensures that the op only gets evaluated once...

@matanster

This comment has been minimized.

matanster commented Sep 22, 2015

Sorry to awaken this old thread.
Thankfully I gather this is solved as of 2.11.6, and indeed I see the overridden function in 2.11.7. However can you explain what exactly is it that makes getOrElseUpdate atomic?

override def getOrElseUpdate(k: K, op: =>V): V = {
    val oldv = lookup(k)
    if (oldv != null) oldv.asInstanceOf[V]
    else {
      val v = op
      if (v == null) {
        throw new NullPointerException("Concurrent TrieMap values cannot be null.")
      } else {
        val hc = computeHash(k)
        insertifhc(k, hc, v, INode.KEY_ABSENT) match {
          case Some(oldv) => oldv
          case None => v
        }
      }
    }
  }

I can imagine one or more race conditions if I take it that only the delegation to insertifhc is atomic; Imagine a call getOrElseUpdate(k, x). Imagine lookup finds a key k is not already stored in the trie, and the code proceeds up until but not including the match statement's right-hand block. Another thread now runs through the full cycle of a call getOrElseUpdate(k, y), inserting some value y != x to the trie, for the same key k. Now executing the match block, the code will return x, whereas the real value now stored is y. What am I missing?

@Ichoran

This comment has been minimized.

Contributor

Ichoran commented Sep 23, 2015

@matanster - I think you are missing what insertifhc returns. Did you check its source code?

@matanster

This comment has been minimized.

matanster commented Sep 23, 2015

Yes, but I was incorrectly interpreting the code. I guess that's why I don't typically write concurrent libraries ;) Shall I remove my unnecessary comment then from this issue here?

@SethTisue

This comment has been minimized.

Member

SethTisue commented Sep 23, 2015

that's not necessary. if you're now convinced the code is correct, this conversation documents that it passed an additional level of review, with at least the three of us, perhaps more, having taken a second look.

finaglehelper pushed a commit to twitter/finatra that referenced this pull request Jun 20, 2016

finatra: Remove com.twitter.inject.conversions.map#atomicGetOrElseUpdate
Problem

This function was added because getOrElseUpdate was not atomic until
Scala 2.11.6. Since we supported 2.10 we included our own simple
atomic implementation.

See: scala/scala#4319

We now no longer support Scala 2.10.

Solution

Delete this function and change callers to use `getOrElseUpdate`
directly since this is now atomic for the supported version of
Scala.

RB_ID=842684
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment