[
https://issues.apache.org/jira/browse/ZOOKEEPER-3652?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17041741#comment-17041741
]
maoling commented on ZOOKEEPER-3652:
------------------------------------
_*--->the value of {{state}} changes over time. Which means that the object on
which we synchronize may not always be the same, thus defeating the purpose of
using synchronization to coordinate access between*_ *threads**.*
I write an example to check this. Yes, I can read *Thread name:A-thread, the
state is:B*
{code:java}
public class EnumTest {
static volatile States state = States.C;
static Object object = new Object();
public enum States {
A,
B,
C;
}
public static void main(String[] args) {
Thread a = new MyThreadA("A-thread");
a.start();
Thread b = new MyThreadB("B-thread");
b.start();
}
static class MyThreadB extends Thread {
private String name;
public MyThreadB(String name) {
this.name = name;
}
Random r = new Random(1);
@Override
public void run() {
while (true) {
synchronized (state) {
state = States.B;
try {
TimeUnit.MILLISECONDS.sleep(r.nextInt(1000));
} catch (InterruptedException e) {
e.printStackTrace();
}
System.out.println("Thread name:" + name + ", the state
is:" + state);
}
try {
TimeUnit.MILLISECONDS.sleep(r.nextInt(1000));
} catch (InterruptedException e) {
e.printStackTrace();
}
}
}
}
static class MyThreadA extends Thread {
private String name;
public MyThreadA(String name) {
this.name = name;
}
Random r = new Random(1);
@Override
public void run() {
while (true) {
synchronized (state) {
state = States.A;
try {
TimeUnit.MILLISECONDS.sleep(r.nextInt(1000));
} catch (InterruptedException e) {
e.printStackTrace();
}
System.out.println("Thread name:" + name + ", the state
is:" + state);
}
try {
TimeUnit.MILLISECONDS.sleep(r.nextInt(1000));
} catch (InterruptedException e) {
e.printStackTrace();
}
}
}
}
}
{code}
[~sylvain]
Could you please help us improve this?
Zookeeper uses the github workflow. The contributor guideline
is
[here](https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute])
> Improper synchronization in ClientCnxn
> --------------------------------------
>
> Key: ZOOKEEPER-3652
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-3652
> Project: ZooKeeper
> Issue Type: Bug
> Components: java client
> Affects Versions: 3.5.6
> Reporter: Sylvain Wallez
> Priority: Major
>
> ZOOKEEPER-2111 introduced {{synchronized(state)}} statements in
> {{ClientCnxn}} and {{ClientCnxn.SendThread}} to coordinate insertion in
> {{outgoingQueue}} and draining it when the client connection isn't alive.
> There are several issues with this approach:
> - the value of the {{state}} field is not stable, meaning we don't always
> synchronize on the same object.
> - the {{state}} field is an enum value, which are global objects. So in an
> application with several ZooKeeper clients connected to different servers,
> this causes some contention between clients.
> An easy fix is change those {{synchronized(state)}} statements to
> {{synchronized(outgoingQueue)}} since it is local to each client and is what
> we want to coordinate.
> I'll be happy to prepare a PR with the above change if this is deemed to be
> the correct way to fix it.
>
> Another issue that makes contention worse is
> {{ClientCnxnSocketNIO.cleanup()}} that is called from within the above
> synchronized block and contains {{Thread.sleep(100)}}. Why is this sleep
> statement needed, and can we remove it?
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
