[
https://issues.apache.org/jira/browse/SLIDER-1155?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15378740#comment-15378740
]
Gour Saha commented on SLIDER-1155:
-----------------------------------
I reviewed the code for all the issues listed here. In all the
ConcurrentHashMap and AtomicBoolean cases, there are either multiple operations
being performed (multiple internal states being modified) within the block or
non-map related work being done like wait and notify. There is documentation
also in certain areas of the code which states specifically why double
synchronization is being done.
The DateFormat is a false positive.
In fact making the code changes as per suggestions causes several tests to
fail, which indicates that tests were written specifically for the way the code
exists today.
Following are the tests which fail when code changes are made as per the above
guidelines -
{code}
TestWorkflowForkedProcessService.testLs:76->Assert.assertNull:656->Assert.assertNull:646->Assert.failNotNull:664->Assert.fail:88
expected null, but was:<java.lang.IllegalMonitorStateException>
TestFreezeCommands.testFreezeCommands:169->SliderTestUtils.execSliderCommand:1004->Object.wait:-2
» IllegalMonitorState
TestStandaloneAgentAM.testStandaloneAgentAM:204->Object.wait:-2 »
IllegalMonitorState
TestStandaloneAMDestroy.testStandaloneAMDestroy:137->Object.wait:-2 »
IllegalMonitorState
TestZKIntegration.testListUserClustersWithTwoCluster:95->initZKI:65->YarnZKMiniClusterTestBase.createZKIntegrationInstance:-1->YarnZKMiniClusterTestBase.createZKIntegrationInstance:67->Object.wait:-2
» IllegalMonitorState
TestZKIntegration.testListUserClustersWithoutAnyClusters:72->initZKI:65->YarnZKMiniClusterTestBase.createZKIntegrationInstance:-1->YarnZKMiniClusterTestBase.createZKIntegrationInstance:67->Object.wait:-2
» IllegalMonitorState
TestZKIntegration.testCreateAndDeleteDefaultZKPath:118->YarnZKMiniClusterTestBase.createZKIntegrationInstance:-1->YarnZKMiniClusterTestBase.createZKIntegrationInstance:67->Object.wait:-2
» IllegalMonitorState
TestZKIntegration.testListUserClustersWithOneCluster:82->initZKI:65->YarnZKMiniClusterTestBase.createZKIntegrationInstance:-1->YarnZKMiniClusterTestBase.createZKIntegrationInstance:67->Object.wait:-2
» IllegalMonitorState
{code}
Having said that, there definitely lies scope to improve the code style/pattern
to pass such scans as well.
> Code issues - 16 multithreaded correctness
> ------------------------------------------
>
> Key: SLIDER-1155
> URL: https://issues.apache.org/jira/browse/SLIDER-1155
> Project: Slider
> Issue Type: Bug
> Components: other
> Affects Versions: Slider 0.91
> Reporter: Gour Saha
> Assignee: Gour Saha
> Fix For: Slider 1.0.0
>
>
> Following 16 possible multithreaded correctness issues need to be evaluated
> and fixed.
> h6.
> slider-core/src/main/java/org/apache/slider/server/services/workflow/ForkedProcessService.java
> {code}
> 244 synchronized (processTerminated) {
> {code}
> defect: Synchronization performed on java.util.concurrent.atomic.AtomicBoolean
> {code}
> 213 synchronized (processTerminated) {
> {code}
> defect: Synchronization performed on java.util.concurrent.atomic.AtomicBoolean
> h6.
> slider-core/src/main/java/org/apache/slider/server/appmaster/state/RoleStatus.java
> {code}
> 320 public long getCompleted() {
> {code}
> defect: org.apache.slider.server.appmaster.state.RoleStatus.getCompleted() is
> unsynchronized,
> org.apache.slider.server.appmaster.state.RoleStatus.setCompleted(int) is
> synchronized.
> h6.
> slider-core/src/main/java/org/apache/slider/server/appmaster/state/ProviderAppState.java
> {code}
> 94 synchronized (publishedConfigSets) {
> {code}
> defect: Synchronization performed on java.util.concurrent.ConcurrentHashMap
> {code}
> 110 synchronized (publishedConfigSets) {
> {code}
> defect: Synchronization performed on java.util.concurrent.ConcurrentHashMap
> h6.
> slider-core/src/main/java/org/apache/slider/server/appmaster/management/RecordedEvent.java
> {code}
> 54 this.time = timestamp > 0 ? dateFormat.format(timestamp) : "";
> {code}
> defect: Call to method of static java.text.DateFormat
> h6.
> slider-core/src/main/java/org/apache/slider/server/appmaster/management/RangeLimitedCounter.java
> {code}
> 77 public long get() {
> {code}
> defect:
> org.apache.slider.server.appmaster.management.RangeLimitedCounter.get() is
> unsynchronized,
> org.apache.slider.server.appmaster.management.RangeLimitedCounter.set(long)
> is synchronized.
> h6.
> slider-core/src/main/java/org/apache/slider/server/appmaster/SliderAppMaster.java
> {code}
> 1532 isAMCompleted.awaitUninterruptibly();
> {code}
> defect: Condition.await() not in loop.
> h6.
> slider-core/src/main/java/org/apache/slider/providers/agent/ComponentTagProvider.java
> {code}
> 119 synchronized (allTags) {
> {code}
> defect: Synchronization performed on java.util.concurrent.ConcurrentHashMap
> {code}
> 56 compTags.put(tempKey, FREE);
> {code}
> defect: Sequence of calls to java.util.concurrent.ConcurrentHashMap may not
> be atomic.
> {code}
> 106 synchronized (compTags) {
> {code}
> defect: Synchronization performed on java.util.concurrent.ConcurrentHashMap
> {code}
> 52 synchronized (compTags) {
> {code}
> defect: Synchronization performed on java.util.concurrent.ConcurrentHashMap
> {code}
> 121 allTags.put(component, new ConcurrentHashMap<String, String>());
> {code}
> defect: Sequence of calls to java.util.concurrent.ConcurrentHashMap may not
> be atomic
> {code}
> 76 synchronized (compTags) {
> {code}
> defect: Synchronization performed on java.util.concurrent.ConcurrentHashMap
> h6. slider-core/src/main/java/org/apache/slider/core/zk/BlockingZKWatcher.java
> {code}
> 39 synchronized (connectedFlag) {
> {code}
> defect: Synchronization performed on java.util.concurrent.atomic.AtomicBoolean
> {code}
> 55 synchronized (connectedFlag) {
> {code}
> defect: Synchronization performed on java.util.concurrent.atomic.AtomicBoolean
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
