Skip to content
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

Merged
merged 16 commits into from
Nov 15, 2016
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add javadocs

* @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 {

Choose a reason for hiding this comment

The 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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These operations are not atomic. Consider usage of AtomicBoolean

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ClosedByInterruptException is thrown this class should not continue work

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmorhun Please address my comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by using CopyOnWriteArrayList

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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benoitf, we have default toString() here, but I think that it is enough. We'll have consumer implementation class name and exception stacktrace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree with Florent - toString here has no sense

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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
Expand Up @@ -34,11 +34,18 @@ public File getFile() {

@Override
public void writeLine(String line) throws IOException {
if (line != null) {
writer.write(line);
try {

Choose a reason for hiding this comment

The 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -19,7 +20,7 @@
* @author Alexander Garagatyi
*/
public interface ProgressMonitor {

Choose a reason for hiding this comment

The 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
Expand Down