You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "magibney (via GitHub)" <gi...@apache.org> on 2023/09/29 18:51:31 UTC
[GitHub] [solr] magibney opened a new pull request, #1964: SOLR-11535: Fix race condition in singleton-per-collection StateWatcher creation
magibney opened a new pull request, #1964:
URL: https://github.com/apache/solr/pull/1964
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-11535: Fix race condition in singleton-per-collection StateWatcher creation [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on PR #1964:
URL: https://github.com/apache/solr/pull/1964#issuecomment-1758451885
Having lived with these changes for a bit, I plan to commit this within the next day or two, pending feedback. Proposed CHANGES.txt entry:
```
* SOLR-11535: Fix race condition in singleton-per-collection StateWatcher creation (Michael Gibney)
```
I'm inclined to put this under "Bugs". Even though it's a bug that often doesn't often manifest in a visible way, the essence of this issue is that ZkStateReader has been leaking redundant `StateWatcher`s that, once present, re-register themselves in perpetuity as long as the collection is still watched ... which definitely sounds to me like a bug.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-11535: Fix race condition in singleton-per-collection StateWatcher creation [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1964:
URL: https://github.com/apache/solr/pull/1964#issuecomment-1912846936
This PR had no visible activity in the past 60 days, labeling it as stale. Any new activity will remove the stale label. To attract more reviewers, please tag someone or notify the dev@solr.apache.org mailing list. Thank you for your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-11535: Fix race condition in singleton-per-collection StateWatcher creation [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #1964:
URL: https://github.com/apache/solr/pull/1964#discussion_r1513378129
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1679,19 +1698,21 @@ public static String getCollectionPath(String coll) {
* @see ZkStateReader#unregisterCore(String)
*/
public void registerCore(String collection) {
- AtomicBoolean reconstructState = new AtomicBoolean(false);
+ AtomicReference<StateWatcher> newWatcherRef = new AtomicReference<>();
collectionWatches.compute(
collection,
(k, v) -> {
if (v == null) {
- reconstructState.set(true);
- v = new StatefulCollectionWatch();
+ StateWatcher stateWatcher = new StateWatcher(collection);
Review Comment:
Curious, is there a reason to prefer this beyond style? Style-wise I somewhat prefer _not_ writing to a volatile variable and then reading back from it ... but because the `StatefulCollectionWatch` instance is not accessible from other threads at this point there's no functional issue with it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-11535: Fix race condition in singleton-per-collection StateWatcher creation [solr]
Posted by "noblepaul (via GitHub)" <gi...@apache.org>.
noblepaul commented on code in PR #1964:
URL: https://github.com/apache/solr/pull/1964#discussion_r1510478108
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1679,19 +1698,21 @@ public static String getCollectionPath(String coll) {
* @see ZkStateReader#unregisterCore(String)
*/
public void registerCore(String collection) {
- AtomicBoolean reconstructState = new AtomicBoolean(false);
+ AtomicReference<StateWatcher> newWatcherRef = new AtomicReference<>();
collectionWatches.compute(
collection,
(k, v) -> {
if (v == null) {
- reconstructState.set(true);
- v = new StatefulCollectionWatch();
+ StateWatcher stateWatcher = new StateWatcher(collection);
Review Comment:
can we replace this with
```
v = new StatefulCollectionWatch(new StateWatcher(collection));
newWatcherRef.set(v.associatedWatcher);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-11535: Fix race condition in singleton-per-collection StateWatcher creation [solr]
Posted by "noblepaul (via GitHub)" <gi...@apache.org>.
noblepaul commented on code in PR #1964:
URL: https://github.com/apache/solr/pull/1964#discussion_r1510478108
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1679,19 +1698,21 @@ public static String getCollectionPath(String coll) {
* @see ZkStateReader#unregisterCore(String)
*/
public void registerCore(String collection) {
- AtomicBoolean reconstructState = new AtomicBoolean(false);
+ AtomicReference<StateWatcher> newWatcherRef = new AtomicReference<>();
collectionWatches.compute(
collection,
(k, v) -> {
if (v == null) {
- reconstructState.set(true);
- v = new StatefulCollectionWatch();
+ StateWatcher stateWatcher = new StateWatcher(collection);
Review Comment:
Can we replace this with
can we replace this with
```
v = new StatefulCollectionWatch(new StateWatcher(collection));
newWatcherRef.set(v.associatedWatcher);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1964: SOLR-11535: Fix race condition in singleton-per-collection StateWatcher creation
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #1964:
URL: https://github.com/apache/solr/pull/1964#discussion_r1341703136
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1329,8 +1336,9 @@ public void process(WatchedEvent event) {
return;
}
- if (!collectionWatches.watchedCollections().contains(coll)) {
- // This collection is no longer interesting, stop watching.
+ StatefulCollectionWatch scw = collectionWatches.statefulWatchesByCollectionName.get(coll);
+ if (scw == null || scw.associatedWatcher != this) {
+ // Collection no longer interesting, or we have been replaced by a different watcher.
Review Comment:
The substance of the fix is in these three lines. Since StateWatcher instance do a deferred check against collectionWatches in order to determine whether to exit, it's possible that by the time a StateWatcher checks here, the watch it was created to serve has been removed, and a new watch inserted (and new StateWatcher created) before this check. In that case it's possible to accumulate persistent StateWatcher instances that all notify callbacks on the same event.
The fix is for the StateWatcher to check whether it is the instance responsible for the current entry in collectionWatches, and exit if not. The rest of the changes are basically just to ensure that The new StatefulCollectionWatch.associatedWatcher field is never null once it's been inserted in the map.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-11535: Fix race condition in singleton-per-collection StateWatcher creation [solr]
Posted by "noblepaul (via GitHub)" <gi...@apache.org>.
noblepaul commented on code in PR #1964:
URL: https://github.com/apache/solr/pull/1964#discussion_r1510479018
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1763,26 +1784,29 @@ public void registerCollectionStateWatcher(
* <p>The Watcher will automatically be removed when it's <code>onStateChanged</code> returns
* <code>true</code>
*/
- public void registerDocCollectionWatcher(String collection, DocCollectionWatcher stateWatcher) {
- AtomicBoolean watchSet = new AtomicBoolean(false);
+ public void registerDocCollectionWatcher(
+ String collection, DocCollectionWatcher docCollectionWatcher) {
+ AtomicReference<StateWatcher> newWatcherRef = new AtomicReference<>();
collectionWatches.compute(
collection,
(k, v) -> {
if (v == null) {
- v = new StatefulCollectionWatch();
- watchSet.set(true);
+ StateWatcher stateWatcher = new StateWatcher(collection);
Review Comment:
same here too
```
v = new StatefulCollectionWatch(new StateWatcher(collection));
newWatcherRef.set(v.associatedWatcher);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-11535: Fix race condition in singleton-per-collection StateWatcher creation [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney merged PR #1964:
URL: https://github.com/apache/solr/pull/1964
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org