-
Notifications
You must be signed in to change notification settings - Fork 658
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
SOLR-16871: Race condition in CoordinatorHttpSolrCall
synthetic collection/replica init
#1762
SOLR-16871: Race condition in CoordinatorHttpSolrCall
synthetic collection/replica init
#1762
Conversation
//and then CoordinatorHttpSolrCall will call getCore again hence creating a calling loop | ||
try { | ||
zkStateReader.waitForState(syntheticCollectionName, 10, TimeUnit.SECONDS, docCollection -> { | ||
for (Replica nodeNameSyntheticReplica : docCollection.getReplicas(solrCall.cores.getZkController().getNodeName())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NULLPTR_DEREFERENCE: List DocCollection.getReplicas(String)
could be null (from the call to DocCollection.getReplicas(...)
on line 139) and is dereferenced.
ℹ️ Expand to see all @sonatype-lift commands
You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.
Command | Usage |
---|---|
@sonatype-lift ignore |
Leave out the above finding from this PR |
@sonatype-lift ignoreall |
Leave out all the existing findings from this PR |
@sonatype-lift exclude <file|issue|path|tool> |
Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file |
Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.
}); | ||
} catch (Exception e) { | ||
throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Failed to wait for active replica for synthetic collection [" + syntheticCollectionName + "]", e); | ||
} | ||
} | ||
core = solrCall.getCoreByCollection(syntheticCollectionName, isPreferLeader); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THREAD_SAFETY_VIOLATION: Unprotected write. Non-private method CoordinatorHttpSolrCall.getCore(...)
indirectly mutates container core.SolrResourceLoader.classNameCache
via call to Map.put(...)
outside of synchronization.
Reporting because a superclass class org.apache.solr.servlet.HttpSolrCall
is annotated @ThreadSafe
, so we assume that this method can run in parallel with other non-private methods in the class (including itself).
ℹ️ Expand to see all @sonatype-lift commands
You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.
Command | Usage |
---|---|
@sonatype-lift ignore |
Leave out the above finding from this PR |
@sonatype-lift ignoreall |
Leave out all the existing findings from this PR |
@sonatype-lift exclude <file|issue|path|tool> |
Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file |
Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.
TimeUnit.SECONDS, | ||
docCollection -> { | ||
for (Replica nodeNameSyntheticReplica : | ||
docCollection.getReplicas(solrCall.cores.getZkController().getNodeName())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NULLPTR_DEREFERENCE: List DocCollection.getReplicas(String)
could be null (from the call to DocCollection.getReplicas(...)
on line 149) and is dereferenced.
ℹ️ Expand to see all @sonatype-lift commands
You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.
Command | Usage |
---|---|
@sonatype-lift ignore |
Leave out the above finding from this PR |
@sonatype-lift ignoreall |
Leave out all the existing findings from this PR |
@sonatype-lift exclude <file|issue|path|tool> |
Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file |
Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@patsonluk I've made the changes to avoid race condition in core creation as well |
@@ -208,9 +208,12 @@ private static void setMDCLoggingContext(String collectionName) { | |||
private static void addReplica(String syntheticCollectionName, CoreContainer cores) { | |||
SolrQueryResponse rsp = new SolrQueryResponse(); | |||
try { | |||
String coreName = syntheticCollectionName + "_" + "r1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to include node name as a part of the core name? otherwise 2 coordinator nodes might use same name for the core?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that. Maybe not required. Core names do not have to be unique
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know! 👍🏼
In this case we might want to always call waitForState
as in https://github.com/apache/solr/pull/1762/files#diff-eedf409265fc219f98f193ae89d3f1b09df78fe49f70bb5b9eaa6c6ff46e6ac7R143 since addReplica
can now return while the target replica is still under construction by another request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I think we do need to add node name, as the AddReplicaCmd logic would iterate through all slices and throw exception on any core name matches.
Therefore, for the 1st coordinator node, it's fine -> the replica is created by the collection creation call, which follows the standard name, ie .sys.COORDINATOR-COLL-conf_shard1_replica_n1
.
The 2nd coordinator node is also fine -> .sys.COORDINATOR-COLL-conf_r1
.
However starting from the 3rd coordinator, it would fail with infinite loop, since it will skip adding replica (due to the check linked above), but solrCall.getCoreByCollection(syntheticCollectionName, isPreferLeader)
would not be able to load such core (since it's not in 3rd node), hence causing the infinite loop.
Such can be reproduced by modifying node count here in the test case from 2 to 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true. I'm not even sure why someone added that check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @noblepaul !
I added another small commit 45654e1 on TestCoordinatorRole
to ensure that the fix work:
- Changed coordinator node count 2 -> 4
- Verified the replica count on the synthetic collection
Also a minor change to the addReplica flow, that we always check for replica status afterwards (since addReplica
might now return if exception is thrown and caught, and with replica status not always active, we could run into infinite call loop, this is a rather rare case, but it doesn't hurt to check.
…nator node Minor fix to synthetic collection addReplica flow to ensure no stack overflow
…lection/replica init (#1762)
@noblepaul @patsonluk, This test has been failing very frequently since merged, did you have time to take a look? |
@tflobbe thanks for raising the concern, do u have any links to failures? Update ah found some in http://fucit.org/solr-jenkins-reports/failure-report.html
|
@noblepaul I think setting the core name does not work as it would not avoid duplicated core on the first coordinator node. I printed out the replica list on failure and it shows:
Any thoughts? 🤔 |
@noblepaul what do u think about this proposed fix? 😊 #1794 |
…lection/replica init (apache#1762)
https://issues.apache.org/jira/browse/SOLR-16871
Description
From a unit test case that issue concurrent select queries to coordinator nodes, it’s found that there could be 3 race condition issues:
If multiple concurrent requests find the synthetic collection is not yet created, they might all attempt to create the synthetic collection. This could trigger SolrException on
collection already exists
Similarly, if multiple concurrent requests find there’s no replica of the synthetic collection for current node (multiple coordinator node scenario), then CoordinatorHttpSolrCall#addReplica could be invoked multiple times. This should not trigger any exception, but would create multiple replicas for the same node in the synthetic collection
The existing logic here assumes if syntheticColl.getReplicas(solrCall.cores.getZkController().getNodeName()) returns non empty result, then the following call in here should return a core. Unfortunately, the first call can return a non empty list but with a DOWN replica if another request is in the progress of creating such replica. In this case, the solrCall.getCoreByCollection(syntheticCollectionName, isPreferLeader) would call super.getCoreByCollection at here which would return a null (since super impl only returns active replica). So CoordinatorHttpSolrCall#getCoreByCollection would end up calling CoordinatorHttpSolrCall#getCore , introducing an infinite loop and cause stack overflow
Solution
zkStateReader.waitForState
. This avoids the infinite loop caused by the presence ofdown
replica.Take note that this does NOT avoid the 2nd issue above, concurrent requests can still create multiple replica for the same node in the synthetic collection, though it's probably benign (and unlikely)
Remarks: First attempt was actually provide proper locking to avoid race condition. However, it's quite tricky to get it right - might need to force refresh the zkReader and do multiple extra reads. The extra cost and complexity probably does not justify the gain.
Tests
Added
TestCooridnatorRole#testConcurrentAccess
to reproduce the issueChecklist
Please review the following and check all that apply:
main
branch../gradlew check
.