[
https://issues.apache.org/jira/browse/HADOOP-3455?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12600641#action_12600641
]
Steve Loughran commented on HADOOP-3455:
----------------------------------------
>> Synchronizing on the (out) stream is dangerous as its value changes during
>> the life of the class [ ...]
>Hmm. I don't think its value actually changes, although the code could make
>this clearer. And moving this synchronization to the connection >seems wrong:
>we need to be able to send and receive messages asynchronously over a
>connection, thus synchronizing separately on its input >and output streams.
How about the class adds a lock field:
private final outputLock=new Object()
and then all synchronized(out) becomes synchronized on outputLock instead? It
would add the scope you want without any risk of NPEs.
> 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.
