Avoid modifying surface cache while updating it

[som-snytt]

Community build scala/community-build#1568 noticed that delegating to ConcurrentHashMap in scala/scala#10027 makes ReflectSurfaceFactory.apply fail due to recursive update of cache.

This commit refactors so that the "update" function returns the surface or null for the LazySurface case. (Where null means nothing to cache.)

I had trouble testing: first to get the build to use my local Scala, which requires setting scalaHome in the subproject settings, then with getting the test run to use my fix in the code base, which I did not resolve. So I have not seen the test go green; the stack trace is the old code. Does cross compilation somehow pick up the old library? I did not investigate further at this point.

[xerial]

@som-snytt Thanks for the pointer to the change of ConcurrentHashMap. I guess it will not be back-ported to Scala 2.12, and will be available in Scala 2.13.10.

A reason you see an old code reference is that AirSpec, a testing library used for testing Airframe, embeds the source code of Surface.

Anyway, the problem is that Surface needs to avoid recursive updates of cache while running getOrElseUpdate. It will be a big change, though.

[som-snytt]

Also, this PR won't work against 2.13.8 because returning null doesn't work for the previous wrapper, which winds up calling putIfAbsent (which raises NPE) instead of computeIfAbsent (which just returns the null).

So maybe that's not the best most practical way to structure the code after all. My first thought was to pass a flag into SurfaceFinder to say "do not attempt to cache the result".

Most of my effort was just trying to run a test, so I ran out of gas on the actual fix. I can give it another try if that helps.

I had not asked if they want to backport the change to the Java wrapper, but that sounds like a good idea; unfortunately it missed 2.12.16.

[xerial]

@som-snytt Let me try a fundamental fix this week by changing the way of caching Surfaces.

[som-snytt]

OK. My other idea is to inline surfaceCache.underlying.computeIfAbsent. (cast to the wrapper required) I'll push that if it works.

Actually, the cast is hindered by

// not private[convert] because `WeakHashMap` uses JMapWrapper
private[collection] object JavaCollectionWrappers extends Serializable {

It should not be private at all, for just this use case! Or all wrappers should extend an interface that exposes the underlying collection.

Edit: that version just demonstrates the other paths to recursion you mentioned.

[xerial]

Applied a potential fix in #2261. I haven't checked this build using a snapshot version of Scala 2.13. Need to wait for the next community build run.

[som-snytt]

@xerial Thanks!