[
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.
