You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2023/01/10 23:31:14 UTC
[GitHub] [druid] github-code-scanning[bot] commented on a diff in pull request #12032: Migrate Druid HttpClient from Netty 3 to 4
github-code-scanning[bot] commented on code in PR #12032:
URL: https://github.com/apache/druid/pull/12032#discussion_r1066442932
##########
core/src/main/java/org/apache/druid/java/util/http/client/response/SequenceInputStreamResponseHandler.java:
##########
@@ -56,18 +56,16 @@
@Override
public ClientResponse<InputStream> handleResponse(HttpResponse response, TrafficCop trafficCop)
{
- try (ChannelBufferInputStream channelStream = new ChannelBufferInputStream(response.getContent())) {
- queue.put(channelStream);
- }
- catch (IOException e) {
- throw new RuntimeException(e);
+ try {
+ // SequenceInputStream constructor blocks if the queue is empty, however no content will be queued until
+ // the first chunk comes in. Adding an empty initial buffer to unblock.
+ queue.put(new ByteBufInputStream(Unpooled.EMPTY_BUFFER)); // lgtm [java/input-resource-leak]
Review Comment:
## Potential input resource leak
This ByteBufInputStream is not always closed on method exit.
[Show more details](https://github.com/apache/druid/security/code-scanning/3638)
##########
core/src/main/java/org/apache/druid/java/util/http/client/response/SequenceInputStreamResponseHandler.java:
##########
@@ -102,21 +100,19 @@
@Override
public ClientResponse<InputStream> handleChunk(
ClientResponse<InputStream> clientResponse,
- HttpChunk chunk,
+ HttpContent chunk,
long chunkNum
)
{
- final ChannelBuffer channelBuffer = chunk.getContent();
- final int bytes = channelBuffer.readableBytes();
+ final ByteBuf byteBuf = chunk.content();
+ final int bytes = byteBuf.readableBytes();
if (bytes > 0) {
- try (ChannelBufferInputStream channelStream = new ChannelBufferInputStream(channelBuffer)) {
- queue.put(channelStream);
+ try {
+ // input streams will be closed by the consumer as we iterate through them in SequenceInputStream
+ queue.put(new ByteBufInputStream(byteBuf.retain(), true)); // lgtm [java/input-resource-leak]
Review Comment:
## Potential input resource leak
This ByteBufInputStream is not always closed on method exit.
[Show more details](https://github.com/apache/druid/security/code-scanning/3639)
##########
core/src/test/java/org/apache/druid/java/util/http/client/FriendlyServersTest.java:
##########
@@ -123,35 +118,30 @@
final ExecutorService exec = Executors.newSingleThreadExecutor();
final ServerSocket serverSocket = new ServerSocket(0);
exec.submit(
- new Runnable()
- {
- @Override
- public void run()
- {
- while (!Thread.currentThread().isInterrupted()) {
- try (
- Socket clientSocket = serverSocket.accept();
- BufferedReader in = new BufferedReader(
- new InputStreamReader(clientSocket.getInputStream(), StandardCharsets.UTF_8)
- );
- OutputStream out = clientSocket.getOutputStream()
- ) {
- StringBuilder request = new StringBuilder();
- String line;
- while (!"".equals((line = in.readLine()))) {
- request.append(line).append("\r\n");
- }
- requestContent.set(request.toString());
- out.write("HTTP/1.1 200 OK\r\n\r\n".getBytes(StandardCharsets.UTF_8));
-
- while (!in.readLine().equals("")) {
- // skip lines
- }
- out.write("HTTP/1.1 200 OK\r\nContent-Length: 6\r\n\r\nhello!".getBytes(StandardCharsets.UTF_8));
+ () -> {
+ while (!Thread.currentThread().isInterrupted()) {
+ try (
+ Socket clientSocket = serverSocket.accept();
+ BufferedReader in = new BufferedReader(
+ new InputStreamReader(clientSocket.getInputStream(), StandardCharsets.UTF_8)
+ );
+ OutputStream out = clientSocket.getOutputStream()
+ ) {
+ StringBuilder request = new StringBuilder();
+ String line;
+ while (!"".equals((line = in.readLine()))) {
Review Comment:
## Inefficient empty string test
Inefficient comparison to empty string, check for zero length instead.
[Show more details](https://github.com/apache/druid/security/code-scanning/3642)
##########
core/src/main/java/org/apache/druid/java/util/http/client/response/SequenceInputStreamResponseHandler.java:
##########
@@ -136,19 +132,14 @@
try {
// An empty byte array is put at the end to give the SequenceInputStream.close() as something to close out
// after done is set to true, regardless of the rest of the stream's state.
- queue.put(ByteSource.empty().openStream());
+ queue.put(new ByteBufInputStream(Unpooled.EMPTY_BUFFER, true)); // lgtm [java/input-resource-leak]
Review Comment:
## Potential input resource leak
This ByteBufInputStream is not always closed on method exit.
[Show more details](https://github.com/apache/druid/security/code-scanning/3640)
##########
core/src/test/java/org/apache/druid/java/util/http/client/FriendlyServersTest.java:
##########
@@ -253,6 +246,39 @@
}
}
+ @Nonnull
+ private AtomicReference<String> acceptEncodingServer(ExecutorService exec, ServerSocket serverSocket)
+ {
+ final AtomicReference<String> foundAcceptEncoding = new AtomicReference<>(null);
+
+ exec.submit(
+ () -> {
+ while (!Thread.currentThread().isInterrupted()) {
+ try (
+ Socket clientSocket = serverSocket.accept();
+ BufferedReader in = new BufferedReader(
+ new InputStreamReader(clientSocket.getInputStream(), StandardCharsets.UTF_8)
+ );
+ OutputStream out = clientSocket.getOutputStream()
+ ) {
+ // Read headers
+ String header;
+ while (!(header = in.readLine()).equals("")) {
Review Comment:
## Inefficient empty string test
Inefficient comparison to empty string, check for zero length instead.
[Show more details](https://github.com/apache/druid/security/code-scanning/3644)
##########
core/src/test/java/org/apache/druid/java/util/http/client/FriendlyServersTest.java:
##########
@@ -123,35 +118,30 @@
final ExecutorService exec = Executors.newSingleThreadExecutor();
final ServerSocket serverSocket = new ServerSocket(0);
exec.submit(
- new Runnable()
- {
- @Override
- public void run()
- {
- while (!Thread.currentThread().isInterrupted()) {
- try (
- Socket clientSocket = serverSocket.accept();
- BufferedReader in = new BufferedReader(
- new InputStreamReader(clientSocket.getInputStream(), StandardCharsets.UTF_8)
- );
- OutputStream out = clientSocket.getOutputStream()
- ) {
- StringBuilder request = new StringBuilder();
- String line;
- while (!"".equals((line = in.readLine()))) {
- request.append(line).append("\r\n");
- }
- requestContent.set(request.toString());
- out.write("HTTP/1.1 200 OK\r\n\r\n".getBytes(StandardCharsets.UTF_8));
-
- while (!in.readLine().equals("")) {
- // skip lines
- }
- out.write("HTTP/1.1 200 OK\r\nContent-Length: 6\r\n\r\nhello!".getBytes(StandardCharsets.UTF_8));
+ () -> {
+ while (!Thread.currentThread().isInterrupted()) {
+ try (
+ Socket clientSocket = serverSocket.accept();
+ BufferedReader in = new BufferedReader(
+ new InputStreamReader(clientSocket.getInputStream(), StandardCharsets.UTF_8)
+ );
+ OutputStream out = clientSocket.getOutputStream()
+ ) {
+ StringBuilder request = new StringBuilder();
+ String line;
+ while (!"".equals((line = in.readLine()))) {
+ request.append(line).append("\r\n");
}
- catch (Exception e) {
- Assert.fail(e.toString());
+ requestContent.set(request.toString());
+ out.write("HTTP/1.1 200 OK\r\n\r\n".getBytes(StandardCharsets.UTF_8));
+
+ while (!in.readLine().equals("")) {
Review Comment:
## Inefficient empty string test
Inefficient comparison to empty string, check for zero length instead.
[Show more details](https://github.com/apache/druid/security/code-scanning/3643)
##########
core/src/test/java/org/apache/druid/java/util/http/client/FriendlyServersTest.java:
##########
@@ -66,27 +66,22 @@
final ExecutorService exec = Executors.newSingleThreadExecutor();
final ServerSocket serverSocket = new ServerSocket(0);
exec.submit(
- new Runnable()
- {
- @Override
- public void run()
- {
- while (!Thread.currentThread().isInterrupted()) {
- try (
- Socket clientSocket = serverSocket.accept();
- BufferedReader in = new BufferedReader(
- new InputStreamReader(clientSocket.getInputStream(), StandardCharsets.UTF_8)
- );
- OutputStream out = clientSocket.getOutputStream()
- ) {
- while (!in.readLine().equals("")) {
- // skip lines
- }
- out.write("HTTP/1.1 200 OK\r\nContent-Length: 6\r\n\r\nhello!".getBytes(StandardCharsets.UTF_8));
- }
- catch (Exception e) {
- // Suppress
+ () -> {
+ while (!Thread.currentThread().isInterrupted()) {
+ try (
+ Socket clientSocket = serverSocket.accept();
+ BufferedReader in = new BufferedReader(
+ new InputStreamReader(clientSocket.getInputStream(), StandardCharsets.UTF_8)
+ );
+ OutputStream out = clientSocket.getOutputStream()
+ ) {
+ while (!in.readLine().equals("")) {
Review Comment:
## Inefficient empty string test
Inefficient comparison to empty string, check for zero length instead.
[Show more details](https://github.com/apache/druid/security/code-scanning/3641)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org