Skip to content
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

2.13: new Scala SHA #1568

Merged
merged 7 commits into from Jun 23, 2022
Merged

Conversation

@SethTisue
Copy link
Member Author

[airframe:error] java.lang.IllegalStateException: Recursive update
[airframe:error] 	at java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1760)
[airframe:error] 	at scala.collection.convert.JavaCollectionWrappers$JConcurrentMapWrapper.getOrElseUpdate(JavaCollectionWrappers.scala:442)
[airframe:error] 	at wvlet.airframe.surface.reflect.ReflectSurfaceFactory$SurfaceFinder.surfaceOf(ReflectSurfaceFactory.scala:337)
[airframe:error] 	at wvlet.airframe.surface.reflect.ReflectSurfaceFactory$.$anonfun$apply$1(ReflectSurfaceFactory.scala:136)
[airframe:error] 	at scala.collection.convert.JavaCollectionWrappers$JConcurrentMapWrapper.$anonfun$getOrElseUpdate$2(JavaCollectionWrappers.scala:442)
[airframe:error] 	at java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1705)
[airframe:error] 	at scala.collection.convert.JavaCollectionWrappers$JConcurrentMapWrapper.getOrElseUpdate(JavaCollectionWrappers.scala:442)
[airframe:error] 	at wvlet.airframe.surface.reflect.ReflectSurfaceFactory$.apply(ReflectSurfaceFactory.scala:136)

@xerial @som-snytt this looks like a conflict with scala/scala#10027 — do either of you have an opinion whether the PR needs amending, or whether airframe should be changed?

@som-snytt
Copy link

  def apply(tpe: ru.Type): Surface = {
    surfaceCache.getOrElseUpdate(fullTypeNameOf(tpe), new SurfaceFinder().surfaceOf(tpe))
  }

where surfaceOf

          // Cache if not yet cached
          surfaceCache.getOrElseUpdate(fullName, surface)

ConcurrentHashMap#computeIfAbsent says

The mapping function must not modify this map during computation.

So, yes, it must be amended somehow.

I suppose that if you're doing this atomic update, you'd need a flag to say the operation must not attempt caching.

@som-snytt
Copy link

I'll attempt a PR. Wish me luck.

@som-snytt
Copy link

That was harder than expected for sbt reasons. Partial result at wvlet/airframe#2260

@som-snytt
Copy link

@SethTisue I see that the problem with my fix is that getOrElseUpdate calls putIfAbsent (which NPEs on null) in 2.13.8, vs computeIfAbsent (ignore if null) in the fixed version.

In other words, the release notes should highlight that change. I'll edit the PR comment.

Although airframe has to make more than the local change I proposed, I see that it would be convenient if clients of the wrapper could access the underlying java collection, even if that required a cast to some well-known interface.

@xerial
Copy link
Contributor

xerial commented Jun 21, 2022

I've made a potential fix to airframe's master branch. Let me know if the build still fails.

@SethTisue SethTisue force-pushed the new-scala-2.13-sha-20220615 branch from b59388c to 954ed3d Compare June 21, 2022 14:44
@SethTisue
Copy link
Member Author

@xerial new failure log at https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/3753/artifact/logs/airframe-build.log

@SethTisue
Copy link
Member Author

(note that progress was made — it no longer hangs the build)

@xerial
Copy link
Contributor

xerial commented Jun 21, 2022

Thanks for the update. I'll check these failures at wvlet/airframe#2265

@SethTisue
Copy link
Member Author

@xerial almost there!

[airframe] [error] Error: Total 40, Failed 0, Errors 2, Passed 37, Skipped 1, Pending 1
[airframe] [error] Error during tests:
[airframe] [error] 	wvlet.airframe.http.openapi.SimpleOpenAPITest
[airframe] [error] (httpCodeGen / Test / test) sbt.TestsFailedException: Tests unsuccessful

https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/3758/artifact/logs/airframe-build.log

@xerial
Copy link
Contributor

xerial commented Jun 22, 2022

Applied a fix.

@SethTisue
Copy link
Member Author

[info] Project airframe--------------------: SUCCESS (project rebuilt ok)

@SethTisue SethTisue merged commit a7e49fc into scala:2.13.x Jun 23, 2022
@SethTisue SethTisue deleted the new-scala-2.13-sha-20220615 branch June 23, 2022 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants