[
https://issues.apache.org/jira/browse/LUCENE-9306?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17076723#comment-17076723
]
Lucene/Solr QA commented on LUCENE-9306:
----------------------------------------
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
|| || || || {color:brown} Prechecks {color} ||
| {color:red}-1{color} | {color:red} test4tests {color} | {color:red} 0m
0s{color} | {color:red} The patch doesn't appear to include any new or modified
tests. Please justify why no new tests are needed for this patch. Also please
list what manual steps were performed to verify this patch. {color} |
|| || || || {color:brown} master Compile Tests {color} ||
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
38s{color} | {color:green} master passed {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m
2s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 1m
2s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} Release audit (RAT) {color} |
{color:green} 1m 2s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} Check forbidden APIs {color} |
{color:green} 1m 2s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} Validate source patterns {color} |
{color:green} 1m 2s{color} | {color:green} the patch passed {color} |
|| || || || {color:brown} Other Tests {color} ||
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 0m
25s{color} | {color:green} replicator in the patch passed. {color} |
| {color:black}{color} | {color:black} {color} | {color:black} 3m 56s{color} |
{color:black} {color} |
\\
\\
|| Subsystem || Report/Notes ||
| JIRA Issue | LUCENE-9306 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12999164/LUCENE-9306.patch |
| Optional Tests | compile javac unit ratsources checkforbiddenapis
validatesourcepatterns |
| uname | Linux lucene2-us-west.apache.org 4.4.0-170-generic #199-Ubuntu SMP
Thu Nov 14 01:45:04 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | ant |
| Personality |
/home/jenkins/jenkins-slave/workspace/PreCommit-LUCENE-Build/sourcedir/dev-tools/test-patch/lucene-solr-yetus-personality.sh
|
| git revision | master / 9b6e072 |
| ant | version: Apache Ant(TM) version 1.9.6 compiled on July 20 2018 |
| Default Java | LTS |
| Test Results |
https://builds.apache.org/job/PreCommit-LUCENE-Build/261/testReport/ |
| modules | C: lucene/replicator U: lucene/replicator |
| Console output |
https://builds.apache.org/job/PreCommit-LUCENE-Build/261/console |
| Powered by | Apache Yetus 0.7.0 http://yetus.apache.org |
This message was automatically generated.
> The check against null and usage of the field updateThread is typically
> protected by synchronization, but not in 1 location.
> ----------------------------------------------------------------------------------------------------------------------------
>
> Key: LUCENE-9306
> URL: https://issues.apache.org/jira/browse/LUCENE-9306
> Project: Lucene - Core
> Issue Type: Bug
> Components: modules/replicator
> Reporter: Adrian Nistor
> Priority: Major
> Attachments: LUCENE-9306.patch
>
>
> h1. Github Pull Request
> I created a pull request on github for this issue at:
> [https://github.com/apache/lucene-solr/pull/1413]
> h1. Description
> Checks against {{null}} and usages of the field {{updateThread}} are
> typically made atomic by synchronization on {{this}} (synchronized methods),
> e.g., at lines:
> [339-341|https://github.com/apache/lucene-solr/blob/531015245042507d71845b6d584e7e7389303093/lucene/replicator/src/java/org/apache/lucene/replicator/ReplicationClient.java#L339-L341],
>
> [357-358|https://github.com/apache/lucene-solr/blob/531015245042507d71845b6d584e7e7389303093/lucene/replicator/src/java/org/apache/lucene/replicator/ReplicationClient.java#L357-L358],
> and
> [380-381|https://github.com/apache/lucene-solr/blob/531015245042507d71845b6d584e7e7389303093/lucene/replicator/src/java/org/apache/lucene/replicator/ReplicationClient.java#L380-L381].
> However, the {{null}} check at [line
> 387|https://github.com/apache/lucene-solr/blob/531015245042507d71845b6d584e7e7389303093/lucene/replicator/src/java/org/apache/lucene/replicator/ReplicationClient.java#L387]
> and the usage at [line
> 388|https://github.com/apache/lucene-solr/blob/531015245042507d71845b6d584e7e7389303093/lucene/replicator/src/java/org/apache/lucene/replicator/ReplicationClient.java#L388]
> are not protected by {{synchronized}}.
> {code:java}
> public String toString() {
> String res = "ReplicationClient";
> if (updateThread != null) { <<<<<<<<<<<<<<<<<<<<<
> res += " (" + updateThread.getName() + ")"; <<<<<<<<<<<<<<<<<
> {code}
> This check against {{null}} and usage of {{updateThread}} are in
> {{toString()}}.
> However, the problem is not that the {{toString()}} will give a garbled
> string (i.e., a relatively minor issues).
> The problem is that, in between the {{null}} check and the usage,
> {{updateThread}} can be set to {{null}} (e.g., by [line
> 369|https://github.com/apache/lucene-solr/blob/531015245042507d71845b6d584e7e7389303093/lucene/replicator/src/java/org/apache/lucene/replicator/ReplicationClient.java#L369])
> and therefore the code can crash.
> I.e., without {{synchronized}}, the {{null}} check does not protect the
> {{updateThread.getName()}} usage.
> I don't know how {{toString()}} is called concurrently. However, it seems
> like a dangerous assumption to make that the callers of {{toString()}} know
> it should not be called concurrently with [line
> 369|https://github.com/apache/lucene-solr/blob/531015245042507d71845b6d584e7e7389303093/lucene/replicator/src/java/org/apache/lucene/replicator/ReplicationClient.java#L369],
> especially as the other methods do protect with synchronization the {{null}}
> check and usage.
> h1. This Patch's Code
> The fix is very simple: just make the method containing [lines
> 387-388|https://github.com/apache/lucene-solr/blob/531015245042507d71845b6d584e7e7389303093/lucene/replicator/src/java/org/apache/lucene/replicator/ReplicationClient.java#L387-L388]
> {{synchronized}}, just like the other methods containing {{null}} checks and
> usages of {{updateThread}}.
> {code:java}
> public synchronized String toString() { <<<<<<<<<<<<<< added "synchronized"
> here
> String res = "ReplicationClient";
> if (updateThread != null) {
> res += " (" + updateThread.getName() + ")";
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
