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

Use LazyThreadSafteyMode.PUBLICATION instead of NONE #433

Merged
merged 1 commit into from
Sep 8, 2021

Conversation

dlurton
Copy link
Member

@dlurton dlurton commented Sep 7, 2021

Issue #367

LazyThreadSafetyMode.NONE literally means that accessing the lazy value from multiple threads can cause an NPE.

Unfortunately, we assumed NONE would be implemented such that a lazy value could be initialized by multiple
threads without throwing an NPE, with race conditions only possibly occurring inside the initializer block.
However, that is actually the behavior is for LazyThreadSafeteyMode.PUBLICATION.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

LazyThreadSafetyMode.NONE literally means that accessing
the lazy value from multiple threads can cause an NPE.

Unfortunately, we assumed NONE would be implemented such
that a lazy value could be initialized by multiple
threads without throwing an NPE, with race conditions
only possibly occurring inside the initializer block.
However, that is actually the behavior is for
LazyThreadSafeteyMode.PUBLICATION.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@codecov-commenter
Copy link

Codecov Report

Merging #433 (90f86be) into main (e64c9a7) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #433   +/-   ##
=========================================
  Coverage     82.33%   82.33%           
  Complexity     1396     1396           
=========================================
  Files           171      171           
  Lines         10724    10724           
  Branches       1768     1768           
=========================================
  Hits           8830     8830           
  Misses         1353     1353           
  Partials        541      541           
Flag Coverage Δ
CLI 18.18% <ø> (ø)
EXAMPLES 74.85% <ø> (ø)
LANG 84.83% <100.00%> (ø)
PTS ∅ <ø> (∅)
TEST_SCRIPT 79.68% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lang/src/org/partiql/lang/eval/Bindings.kt 71.42% <100.00%> (ø)
lang/src/org/partiql/lang/eval/ExprValueFactory.kt 91.07% <100.00%> (ø)
...pt/src/org/partiql/testscript/compiler/Compiler.kt 87.25% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e64c9a7...90f86be. Read the comment docs.

@dlurton
Copy link
Member Author

dlurton commented Sep 7, 2021

FWIW, without this fix I was able to reproduce the issue ~10% of the time with this code, but was never able to reproduce it with the fix.

@dlurton
Copy link
Member Author

dlurton commented Sep 7, 2021

I should add; given the inconsistency of the test in the gist, I don't think we should add that test to our suite.

@dlurton dlurton merged commit eef183c into main Sep 8, 2021
@dlurton dlurton deleted the lazy-publication branch September 8, 2021 00:01
alancai98 pushed a commit that referenced this pull request Sep 13, 2021
LazyThreadSafetyMode.NONE literally means that accessing
the lazy value from multiple threads can cause an NPE.

Unfortunately, we assumed NONE would be implemented such
that a lazy value could be initialized by multiple
threads without throwing an NPE, with race conditions
only possibly occurring inside the initializer block.
However, that is actually the behavior is for
LazyThreadSafeteyMode.PUBLICATION.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

(cherry picked from commit eef183c)
alancai98 pushed a commit that referenced this pull request Sep 13, 2021
LazyThreadSafetyMode.NONE literally means that accessing
the lazy value from multiple threads can cause an NPE.

Unfortunately, we assumed NONE would be implemented such
that a lazy value could be initialized by multiple
threads without throwing an NPE, with race conditions
only possibly occurring inside the initializer block.
However, that is actually the behavior is for
LazyThreadSafeteyMode.PUBLICATION.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

(cherry picked from commit eef183c)
alancai98 added a commit that referenced this pull request Sep 13, 2021
LazyThreadSafetyMode.NONE literally means that accessing
the lazy value from multiple threads can cause an NPE.

Unfortunately, we assumed NONE would be implemented such
that a lazy value could be initialized by multiple
threads without throwing an NPE, with race conditions
only possibly occurring inside the initializer block.
However, that is actually the behavior is for
LazyThreadSafeteyMode.PUBLICATION.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

(cherry picked from commit eef183c)
alancai98 added a commit that referenced this pull request Sep 13, 2021
LazyThreadSafetyMode.NONE literally means that accessing
the lazy value from multiple threads can cause an NPE.

Unfortunately, we assumed NONE would be implemented such
that a lazy value could be initialized by multiple
threads without throwing an NPE, with race conditions
only possibly occurring inside the initializer block.
However, that is actually the behavior is for
LazyThreadSafeteyMode.PUBLICATION.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

(cherry picked from commit eef183c)
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.

Consider using LazyThreadSafetyMode.PUBLICATION instead of LazyThreadSafteyMode.NONE
3 participants