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.

Sign up for GitHub

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

Jump to bottom

Limit exposure to ConcurrentModificationException when sys props are replaced or mutated #22180

Merged
merged 2 commits into from
Dec 20, 2024
Merged

Limit exposure to ConcurrentModificationException when sys props are replaced or mutated #22180

merged 2 commits into from
Dec 20, 2024

Conversation

jtjeferreira
Copy link
Contributor

@jtjeferreira
Copy link
Contributor Author

I think the build error is unrelated

[info] [republish] Fetching org.scala-lang:scala3-interfaces:3.6.4-RC1-bin-SNAPSHOT with coursier.jar...
[info] [republish] Fetching org.scala-lang:scala3-compiler_3:3.6.4-RC1-bin-SNAPSHOT with coursier.jar...
Resolution error: Error downloading org.scala-lang.modules:scala-asm:9.7.0-scala-2
  download error: Caught java.net.ConnectException (Connection timed out) while downloading https://repo1.maven.org/maven2/org/scala-lang/modules/scala-asm/9.7.0-scala-2/scala-asm-9.7.0-scala-2.pom
  not found: /__w/scala3/scala3/dist/linux-x86_64/target/streams/_global/republishAllResolved/_global/streams/localRepo/maven2/org/scala-lang/modules/scala-asm/9.7.0-scala-2/scala-asm-9.7.0-scala-2.pom
Error:  Error running coursier.jar with args fetch --no-default --repository central --repository file:///__w/scala3/scala3/dist/linux-x86_64/target/streams/_global/republishAllResolved/_global/streams/localRepo/maven2 org.scala-lang:scala3-compiler_3:3.6.4-RC1-bin-SNAPSHOT
Error:  (dist-linux-x86_64 / republishAllResolved) Error running coursier.jar with args fetch --no-default --repository central --repository file:///__w/scala3/scala3/dist/linux-x86_64/target/streams/_global/republishAllResolved/_global/streams/localRepo/maven2 org.scala-lang:scala3-compiler_3:3.6.4-RC1-bin-SNAPSHOT
Error:  Total time: 362 s (06:02), completed Dec 10, 2024, 12:40:45 PM
Error: Process completed with exit code 1.

jtjeferreira
jtjeferreira commented Dec 10, 2024
View reviewed changes
compiler/src/dotty/tools/dotc/config/PathResolver.scala
Comment on lines +56 to +57
//using propOrNone/getOrElse instead of propOrElse so that searchForBootClasspath is lazy evaluated
def javaBootClassPath: String = propOrNone("sun.boot.class.path") getOrElse searchForBootClasspath
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this change is a second commit, that is not part of the initial one that I was porting. But I think this improvement makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not an improvement. propOrElse("user.dir", ???). This is the property that was removed at some point, I don't remember the details or naming. I see it is in JDK 8 but not now.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, scala 2 at least supplied scala.boot.class.path as a workaround.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not an improvement

propOrElse is not lazy like in scala2, right? So this was an improvement...

Copy link
Contributor

@som-snytt som-snytt Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's a shame. Edit: I meant the usual lack of code sharing, not the improvement!

eed3si9n
eed3si9n approved these changes Dec 10, 2024
View reviewed changes
Copy link
Member

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

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

@jtjeferreira Thanks!

@adpi2 adpi2 mentioned this pull request Dec 16, 2024
Open
@SethTisue SethTisue self-assigned this Dec 19, 2024
@SethTisue
Copy link
Member

retrying what I hope will prove to be a transient CI failure on Windows

jtjeferreira
jtjeferreira commented Dec 19, 2024
View reviewed changes
compiler/src/dotty/tools/dotc/config/PathResolver.scala
Comment on lines +39 to +48
private def searchForBootClasspath = {
import scala.jdk.CollectionConverters.*
val props = System.getProperties
// This formulation should be immune to ConcurrentModificationExceptions when system properties
// we're unlucky enough to witness a partially published result of System.setProperty or direct
// mutation of the System property map. stringPropertyNames internally uses the Enumeration interface,
// rather than Iterator, and this disables the fail-fast ConcurrentModificationException.
val propNames = props.stringPropertyNames()
propNames.asScala collectFirst { case k if k endsWith ".boot.class.path" => props.getProperty(k) } getOrElse ""
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to understand why this fallback exists, but I landed here and that was a dead end. Maybe we could just remove this code and do

def javaBootClassPath: String = propOrElse("sun.boot.class.path", "")

Copy link
Member

@SethTisue SethTisue Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

probably? for now let's go with the tried-and-true

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that this is what I asked in my other comment: it is because scala 2 runner supplied scala.boot.class.path. It would be worth revisiting what is supported under scala-cli. There is still -bootclasspath option. In scala 2, -nobootcp meant don't use -Xbootclasspath and don't supply scala.boot.classpath. Modern class loader hierarchy is quite different. I always use -nobootcp in scala 2.

@SethTisue SethTisue merged commit 0bfa1af into scala:main Dec 20, 2024
29 checks passed
@jtjeferreira
Copy link
Contributor Author

Thanks for merging this. You don't need nothing else from my side to backport this to other scala 3 branches, right?

@tgodzik
Copy link
Contributor

tgodzik commented Dec 20, 2024

Thanks for merging this. You don't need nothing else from my side to backport this to other scala 3 branches, right?

They all end up on out board and we port everything we can to the lts branch and will automatically be used for the next version. We will most likely have it in 3.3.6 and 3.6.4

@SethTisue SethTisue added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Dec 20, 2024
@SethTisue
Copy link
Member

SethTisue commented Dec 20, 2024 •
edited
Loading

(regardless, adding the "backport:nominated" label for extra assurance that it will be backported if at all possible. not actually sure if the label is looked at 😁 )

@jtjeferreira jtjeferreira mentioned this pull request Dec 23, 2024
Open
@WojciechMazur WojciechMazur mentioned this pull request Dec 30, 2024
Merged
WojciechMazur added a commit that referenced this pull request Dec 30, 2024
@WojciechMazur
Backport "Limit exposure to ConcurrentModificationException when sys …
5771b9b 
…props are replaced or mutated" to 3.6 (#22275)

Backports #22180 to the 3.6.3.

PR submitted by the release tooling.
[skip ci]
@WojciechMazur WojciechMazur added this to the 3.6.3 milestone Dec 30, 2024
@WojciechMazur WojciechMazur added backport:done This PR was successfully backported. and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Dec 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@som-snytt som-snytt som-snytt left review comments

@SethTisue SethTisue SethTisue left review comments

@eed3si9n eed3si9n eed3si9n approved these changes

Assignees

@SethTisue SethTisue

Labels
backport:done This PR was successfully backported.
Projects
None yet
Milestone
3.6.3
Development

Successfully merging this pull request may close these issues.

[2.x] ConcurrentModificationException in SbtParser
6 participants
@jtjeferreira @SethTisue @tgodzik @eed3si9n @som-snytt @WojciechMazur