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

GH-2211 Fix DynamicModel upgrade flaky test failure #2227

Merged
merged 2 commits into from May 16, 2020

Conversation

abrokenjester
Copy link
Contributor

@abrokenjester abrokenjester commented May 16, 2020

GitHub issue resolved: #2211

This is a draft branch to address test failure in org.eclipse.rdf4j.model.DynamicModelConcurrentModificationAndUpgradeTest on Github CI. The test succeeds when ran on my local machine so it's flaky (which is not unusual with multithreaded stuff).

I am not immediately spotting a problem with either the test or the implementation, so opening this draft PR to see the problem (hoping the CI build at least consistently fails) and cooperate on a fix.


PR Author Checklist:

  • my pull request is self-contained
  • I've added tests for the changes I made
  • every commit message starts with the issue number (GH-xxxx) followed by a meaningful description of the change
  • every commit has been signed off

Note: we merge all feature pull requests using squash and merge. See RDF4J git merge strategy for more details.

@abrokenjester
Copy link
Contributor Author

abrokenjester commented May 16, 2020

Stacktrace for the thrown exception (when the test does succeed) is this:

java.lang.UnsupportedOperationException
	at java.util.Collections$UnmodifiableMap.put(Collections.java:1459)
	at org.eclipse.rdf4j.model.impl.DynamicModel.lambda$1(DynamicModel.java:268)
	at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
	at java.util.Iterator.forEachRemaining(Iterator.java:116)
	at java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
	at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:482)
	at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472)
	at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708)

This is on trying to add a new statement to the private statements collection, a LinkedHashMap. I wonder if we're hitting a CPU caching issue (we're replacing the existing object reference with a new one when we mark it unmodifiable) , in which case perhaps just marking the field volatile could solve the problem.

@abrokenjester
Copy link
Contributor Author

abrokenjester commented May 16, 2020

Grmbl, of course the build succeeds this time around :/ I'm going to push the volatile fix anyway, and then merge into my other issue branch to see if it solves the build failures in both cases.

@abrokenjester abrokenjester marked this pull request as ready for review May 16, 2020 04:52
@abrokenjester
Copy link
Contributor Author

Both builds now succeed. This may just be luck of course, but I'm cautiously optimistic.

@hmottestad
Copy link
Contributor

Build never failed for me on GitHub CI.

@abrokenjester
Copy link
Contributor Author

abrokenjester commented May 16, 2020

Build never failed for me on GitHub CI.

Master branch is currently unstable. See https://github.com/eclipse/rdf4j/actions?query=workflow%3A%22master+status%22 . I know it only started failing after merging your Changeset improvements, but the test that fails has to do with the DynamicModel upgrade, so I am assuming it's intermittent and the first build just got lucky.

It also caused my PR build for a completely unrelated bug fix to fail by the way, so it's not just the one build, there's definitely something flaky.

@abrokenjester
Copy link
Contributor Author

Unless you have objections I will merge this, and hope that it fixes the problem.

@abrokenjester abrokenjester merged commit eeb56f6 into master May 16, 2020
@abrokenjester abrokenjester deleted the GH-2211-test-failure branch May 16, 2020 07:02
hmottestad added a commit that referenced this pull request May 16, 2020
@hmottestad
Copy link
Contributor

I don't want to use volatile there, for performance, and I think I've found something wrong with the latches in the test.

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.

Parallel upgrading of DynamicModel can cause NullPointerException
2 participants