[
https://issues.apache.org/jira/browse/HADOOP-10940?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14093330#comment-14093330
]
Colin Patrick McCabe commented on HADOOP-10940:
-----------------------------------------------
nit: maxDataLength should be final, since it can't change
{code}
\@InterfaceAudience.Private // ONLY exposed for SaslRpcClient
public static class IpcStreams implements Closeable {
{code}
Is this comment still valid? It looks like even non-SASL clients are now using
{{IpcStreams}}.
{code}
// don't flush! we need to avoid broken pipes if server closes or
// rejects the connection. the perils of multiple sends before a read
// insecure: header+context+call, flush
// secure : header+negotiate, flush, (sasl), context+call, flush
{code}
Hmm. I wonder if we could rephrase this to be clearer. Maybe something like
"At this point, the data is buffered by the output stream. We do not want to
flush yet, since that would generate unnecessary context switches. Another
advantage of deferring the TCP write operation is that we do not get a "broken
pipe" exception if the server closes or rejects the connection at this point."
{code}
// again, don't flush! see writeConnectionHeader
{code}
Do we need this comment here? There wasn't a flush here earlier.
{code}
public void sendRequest(RpcRequestHeaderProto header, Message request,
boolean flush) throws IOException {
try {
header.writeDelimitedTo(dob);
request.writeDelimitedTo(dob);
sendRequest(dob, flush);
} finally {
dob.reset();
}
}
public void sendRequest(DataOutputBuffer buffer, boolean flush)
throws IOException {
out.writeInt(buffer.size()); // total Length
buffer.writeTo(out); // request header + payload
if (flush) {
out.flush();
}
}
{code}
Rather than having a boolean argument, why not just have the callers who want
to flush call {{ioStreams.out.flush()}}? There seems to be no advantage to
folding it into {{sendRequest}}, and it means that we need a comment to explain
the value of the boolean everywhere.
{code}
public boolean useWrap() {
{code}
add VisibleForTesting?
> RPC client does no bounds checking of responses
> -----------------------------------------------
>
> Key: HADOOP-10940
> URL: https://issues.apache.org/jira/browse/HADOOP-10940
> Project: Hadoop Common
> Issue Type: Bug
> Components: ipc
> Affects Versions: 2.0.0-alpha, 3.0.0
> Reporter: Daryn Sharp
> Assignee: Daryn Sharp
> Priority: Critical
> Attachments: HADOOP-10940.patch, HADOOP-10940.patch
>
>
> The rpc client does no bounds checking of server responses. In the case of
> communicating with an older and incompatible RPC, this may lead to OOM issues
> and leaking of resources.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
