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

Make PositionImpl thread-safe #465

Merged
merged 1 commit into from Dec 9, 2017

Conversation

Projects
None yet
4 participants
@eed3si9n
Member

eed3si9n commented Dec 6, 2017

Fixes #464
Fixes sbt/sbt#3623

Currently, compiling a lot of Java code in CI environment causes NullPointerException, which is
suspected of race condition around javax.tools.Diagnostics[S], which is held by PositionImpl and then later accessed by async logging.

This change makes the PositionImpl strict and immutable, and extracts it from a Java Diagnostics object. No other observable changes are introduced besides, hopefully the lack of NPE. Credit on the detective work goes to Lightbend Akka team @ktoso, @2m, and @jrudolph.

@eed3si9n eed3si9n requested review from dwijnand and jvican Dec 6, 2017

@eed3si9n eed3si9n added the in progress label Dec 6, 2017

@dwijnand

LGTM, one question.

}
} catch {
// TODO - catch ReflectiveOperationException once sbt is migrated to JDK7
case ignored: Throwable => None

This comment has been minimized.

@dwijnand

dwijnand Dec 6, 2017

Member

Should we postpone this change for an RC cycle?

@dwijnand

dwijnand Dec 6, 2017

Member

Should we postpone this change for an RC cycle?

This comment has been minimized.

@eed3si9n

eed3si9n Dec 6, 2017

Member

You have a hawk eye. Sure, I'll be fine with moving that change to 1.1.x.

@eed3si9n

eed3si9n Dec 6, 2017

Member

You have a hawk eye. Sure, I'll be fine with moving that change to 1.1.x.

This comment has been minimized.

@dwijnand

dwijnand Dec 6, 2017

Member

It's clearer when ignoring whitespace changes: https://github.com/sbt/zinc/pull/465/files?w=1.

@dwijnand

dwijnand Dec 6, 2017

Member

It's clearer when ignoring whitespace changes: https://github.com/sbt/zinc/pull/465/files?w=1.

@jrudolph

I guess that will fix the bug, but what should really be fixed is the underlying logging infrastructure, otherwise this will come back in another form.

@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Dec 7, 2017

Member

Reviewing this tomorrow.

Member

jvican commented Dec 7, 2017

Reviewing this tomorrow.

@jvican

LGTM, just one minor request and it's good to go.

Make PositionImpl thread-safe
Fixes #464 sbt/sbt#3623

Currently compiling a lot of Java code in CI environment causes NullPointerException, which is
suspected of race condition around `javax.tools.Diagnostics[S]`, which is held by `PositionImpl` and then later accessed by async logging.

This change makes the `PositionImpl` strict and immutable, and extracts it from a Java Diagnostics object. No other observable changes are introduced besides, hopefully the lack of NPE. Credit on the detective work goes to Lightbend Akka team ktoso, 2m, and jrudolph.
@jvican

jvican approved these changes Dec 9, 2017

@dwijnand dwijnand merged commit 3ccb6c4 into sbt:1.0.x Dec 9, 2017

1 check passed

continuous-integration/drone/pr the build was successful
Details

@dwijnand dwijnand removed the in progress label Dec 9, 2017

@eed3si9n eed3si9n deleted the eed3si9n:wip/problem branch Dec 16, 2017

lorandszakacs added a commit to lorandszakacs/monix that referenced this pull request Dec 23, 2017

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