[jira] Updated: (HADOOP-3455) IPC.Client synchronisation looks weak

["Steve Loughran (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/HADOOP-3455?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Steve Loughran updated HADOOP-3455:
-----------------------------------

    Attachment: hadoop-3455.patch

This patch makes client significantly less thread-unsafe, without going overboard in marking everything as synchronized, as that leads to deadlock.

- the shared fields are marked as volatile, have package-scoped synchronized accessors where need be, and do all read-write operations in synchronized blocks. I'd mark the shared fields as private and then non-volatile, if we knew nobody accessed them.

-synchronized(out) have been replaced with synchronized(this); inside a synchronized method the block was removed completely.

- more javadoc comments on synchronization.

-Pulled calls to System.currentTimeMillis() out of the synchronized blocks, where it was simply a matter of pulling the method call up. There's one place where it is more complex, and I have left it in

-improvements to logging in the process.

There's no extra tests here, as xunit tests aren't any good at generating concurrency problems. This code should really be merged into SVN_HEAD and then someone else who is confident they understand the java memory model needs to have a look through the file and see if there are other concurrency issues that still need to be addressed, particularly at shutdown, or when InterruptedExceptions start getting raised.



> IPC.Client synchronisation looks weak
> -------------------------------------
>
>                 Key: HADOOP-3455
>                 URL: https://issues.apache.org/jira/browse/HADOOP-3455
>             Project: Hadoop Core
>          Issue Type: Improvement
>          Components: ipc
>    Affects Versions: 0.18.0
>            Reporter: Steve Loughran
>         Attachments: hadoop-3455.patch
>
>
> Looking at HADOOP-3453 , its clear that Client.java is inconsistently synchronized
> 1. the running and shouldCloseConnection flags are not always read/written in synchronized blocks, even though they are properties used to share information between threads. They should be marked as volatile for access outside synchronized blocks, and all read-check-update operations must be synchronized.
> 2. there are multiple calls to System.currentTimeMillis() in synchronized blocks; this is a slow native operation and should ideally be done unsynchronized.
> 3. Synchronizing on the (out) stream is dangerous as its value changes during the life of the class, and sometimes it is null. These blocks should all synchronize on the Client instead.
> 4.  There are a number of places where InterruptedExceptions are caught and ignored in a sleep-wait loop:
>      } catch (InterruptedException e) {
>       }
>    This isn't dangerous, but it does make the client harder to stop. These code fragments should be looked at carefully.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.