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

Revert "Use Java collections" #539

Merged
merged 2 commits into from
May 7, 2018
Merged

Revert "Use Java collections" #539

merged 2 commits into from
May 7, 2018

Conversation

eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented May 6, 2018

This reverts commit 52959c9.
Fixes #538

The use of java.util.HashMap causes java.util.ConcurrentModificationException on JDK 9 and JDK 10. This is likely because processType recursively end up calling processType while modifying typeCache.

This reverts commit 52959c9.
Fixes sbt#538

The use of `java.util.HashMap` causes `java.util.ConcurrentModificationException` on JDK 9 and JDK 10. This is likely because `processType` recursively end up calling `processType` while modifying `typeCache`.
@eed3si9n eed3si9n requested review from retronym and jvican May 6, 2018 06:55
This reverts commit 3e4b3b4.
@retronym
Copy link
Member

retronym commented May 7, 2018

Sorry about this, I didn't consider the re-entrancy of these caching methods.

s.c.m.HashMap.getOrElseUpdate accomodates such use cases:

  override def getOrElseUpdate(key: K, defaultValue: => V): V = {
    val hash = table.elemHashCode(key)
    val i = table.index(hash)
    val firstEntry = table.findEntry0(key, i)
    if (firstEntry != null) firstEntry.value
    else {
      val table0 = table.table
      val default = defaultValue
      // Avoid recomputing index if the `defaultValue()` hasn't triggered
      // a table resize.
      val newEntryIndex = if (table0 eq table.table) i else table.index(hash)
      val e = table.createNewEntry(key, default)
      // Repeat search
      // because evaluation of `default` can bring entry with `key`
      val secondEntry = table.findEntry0(key, newEntryIndex)
      if (secondEntry == null) table.addEntry0(e, newEntryIndex)
      else secondEntry.value = default
      default
    }
  }

Whereas the Java collections consider it a programming error and fail fast.

@retronym
Copy link
Member

retronym commented May 7, 2018

I believe @jvican is going to try reworking Zinc to compute the hashes directly without first creating this structure, so reverting is and waiting for that work seems like a good course of action.

@eed3si9n
Copy link
Member Author

eed3si9n commented May 7, 2018

I think this mostly means that we need to add automated testing on JDK 10 sooner rather than later.

Copy link
Member

@jvican jvican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe @jvican is going to try reworking Zinc to compute the hashes directly without first creating this structure, so reverting is and waiting for that work seems like a good course of action.

This is indeed correct, I'm working on it.

@jvican jvican merged commit 899d7bd into sbt:1.1.x May 7, 2018
@eed3si9n eed3si9n deleted the wip/538 branch May 7, 2018 15:58
@eed3si9n
Copy link
Member Author

eed3si9n commented May 7, 2018

Thanks for the quick review guys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants