-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CODENVY-587: Handle closed consumers in composite line comsumer #2549
Changes from 3 commits
54a5b81
5466e88
65635ba
32e5728
384ae6f
8d0e6d1
d561cdc
c52f6cb
cce75a4
b1c1f0b
615be4e
2d2817b
c37ce05
73e86a0
37589f6
655c7d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,38 +14,58 @@ | |
import org.slf4j.LoggerFactory; | ||
|
||
import java.io.IOException; | ||
import java.nio.channels.ClosedByInterruptException; | ||
import java.util.ArrayList; | ||
import java.util.Arrays; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.concurrent.CopyOnWriteArrayList; | ||
|
||
/** | ||
* @author andrew00x | ||
* @author Mykola Morhun | ||
*/ | ||
public class CompositeLineConsumer implements LineConsumer { | ||
private static final Logger LOG = LoggerFactory.getLogger(CompositeLineConsumer.class); | ||
|
||
private final LineConsumer[] lineConsumers; | ||
private final List<LineConsumer> lineConsumers; | ||
private volatile boolean isClosed; | ||
|
||
public CompositeLineConsumer(LineConsumer... lineConsumers) { | ||
this.lineConsumers = lineConsumers; | ||
this.lineConsumers = new CopyOnWriteArrayList<>(lineConsumers); | ||
this.isClosed = false; | ||
} | ||
|
||
@Override | ||
public void close() throws IOException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add javadoc that explains behavior of this implementation of consumer for writeLine and close methods |
||
for (LineConsumer lineConsumer : lineConsumers) { | ||
try { | ||
lineConsumer.close(); | ||
} catch (IOException e) { | ||
LOG.error(String.format("An error occurred while closing the line consumer %s", lineConsumer), e); | ||
if (!isClosed) { | ||
isClosed = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These operations are not atomic. Consider usage of AtomicBoolean There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thx |
||
for (LineConsumer lineConsumer : lineConsumers) { | ||
try { | ||
lineConsumer.close(); | ||
} catch (IOException e) { | ||
LOG.error(String.format("An error occurred while closing the line consumer %s", lineConsumer), e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that toString in line consumers returns pointer to memory area. Do we need that in error message? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, because it also returns name of implementation in which problem occurs |
||
} | ||
} | ||
} | ||
} | ||
|
||
@Override | ||
public void writeLine(String line) throws IOException { | ||
for (LineConsumer lineConsumer : lineConsumers) { | ||
try { | ||
lineConsumer.writeLine(line); | ||
} catch (IOException e) { | ||
LOG.error(String.format("An error occurred while writing line to the line consumer %s", lineConsumer), e); | ||
if (!isClosed) { | ||
for (LineConsumer lineConsumer : lineConsumers) { | ||
try { | ||
lineConsumer.writeLine(line); | ||
} catch (ConsumerAlreadyClosedException | ClosedByInterruptException e) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If ClosedByInterruptException is thrown this class should not continue work There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? Only one subconsumer is closed. What is the reason to close others? |
||
lineConsumers.remove(lineConsumer); // consumer is already closed, so we cannot write into it any more | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it an appropriate way to remove from source while you are iterating through it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say a test is required with isClosed and writeLine throwing exceptions There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mmorhun Please address my comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed by using |
||
if (lineConsumers.size() == 0) { // if all consumers are closed then we can close this one | ||
isClosed = true; | ||
} | ||
} catch (IOException e) { | ||
LOG.error(String.format("An error occurred while writing line to the line consumer %s", lineConsumer), e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is LineConsumer always have nice toString() ? Could we say also the line that we tried to log ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @benoitf, we have default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @benoitf, do you think we need to log that? For example, an output of user's command? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe not I was just trying to see which error message could be helpful to debug/understand what was the issue There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @benoitf thank you, sometimes logged information isn't enough, but the problem here is the reason why it happens. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agree with Florent - toString here has no sense There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is still usable, because we will know consumer implementation |
||
} | ||
} | ||
} | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
/******************************************************************************* | ||
* Copyright (c) 2012-2016 Codenvy, S.A. | ||
* All rights reserved. This program and the accompanying materials | ||
* are made available under the terms of the Eclipse Public License v1.0 | ||
* which accompanies this distribution, and is available at | ||
* http://www.eclipse.org/legal/epl-v10.html | ||
* | ||
* Contributors: | ||
* Codenvy, S.A. - initial API and implementation | ||
*******************************************************************************/ | ||
package org.eclipse.che.api.core.util; | ||
|
||
import java.io.IOException; | ||
|
||
/** | ||
* Is thrown as result of attempt to write a line into closed line consumer. | ||
* | ||
* @author Mykola Morhun | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Javadocs |
||
*/ | ||
public class ConsumerAlreadyClosedException extends IOException { | ||
public ConsumerAlreadyClosedException(String message) { | ||
super(message); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,11 +34,18 @@ public File getFile() { | |
|
||
@Override | ||
public void writeLine(String line) throws IOException { | ||
if (line != null) { | ||
writer.write(line); | ||
try { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please check closing status here |
||
if (line != null) { | ||
writer.write(line); | ||
} | ||
writer.write('\n'); | ||
writer.flush(); | ||
} catch (IOException e) { | ||
if("Stream closed".equals(e.getMessage())) { | ||
throw new ConsumerAlreadyClosedException(e.getMessage()); | ||
} | ||
throw e; | ||
} | ||
writer.write('\n'); | ||
writer.flush(); | ||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
*******************************************************************************/ | ||
package org.eclipse.che.plugin.docker.client; | ||
|
||
import org.eclipse.che.api.core.util.ConsumerAlreadyClosedException; | ||
import org.eclipse.che.plugin.docker.client.json.ProgressStatus; | ||
|
||
/** | ||
|
@@ -19,7 +20,7 @@ | |
* @author Alexander Garagatyi | ||
*/ | ||
public interface ProgressMonitor { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Throwing this exception here doesn't make sense because this class doesn't declare any dependency on line consumer |
||
void updateProgress(ProgressStatus currentProgressStatus); | ||
void updateProgress(ProgressStatus currentProgressStatus) throws ConsumerAlreadyClosedException; | ||
|
||
ProgressMonitor DEV_NULL = new ProgressMonitor() { | ||
@Override | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add javadocs