CODENVY-587: Handle closed consumers in composite line comsumer #2549

mmorhun · 2016-09-22T13:49:02Z

What does this PR do?

Fix wrong behaviour of Composite and File line consumer.
Add concurrent versions of Composite and File line consumers.
Add tests.

What issues does this PR fix or reference?

codenvy/codenvy#587

Previous behavior

Composite and File line consumers tried to write into all subconsumers

New behavior

Composite and File line consumers write only to non-closed subconsumers

mmorhun · 2016-09-22T13:52:21Z

@garagatyi @skabashnyuk review please

codenvy-ci · 2016-09-22T16:48:16Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/478/

benoitf · 2016-09-23T07:10:39Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/CompositeLineConsumer.java


    public CompositeLineConsumer(LineConsumer... lineConsumers) {
-        this.lineConsumers = lineConsumers;
+        this.lineConsumers = new ArrayList<>(lineConsumers.length);
+        Arrays.stream(lineConsumers).forEach(this.lineConsumers::add);


is Arrays.asList(lineConsumers) not working ?

@benoitf Arrays.asList returns immutable list, which is not fit here

why it's an issue to have immutable list ?

because we have to remove closed consumers from the list

benoitf · 2016-09-23T07:12:13Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/CompositeLineConsumer.java

+                        isClosed = true;
+                    }
+                } catch (IOException e) {
+                    LOG.error(String.format("An error occurred while writing line to the line consumer %s", lineConsumer), e);


is LineConsumer always have nice toString() ? Could we say also the line that we tried to log ?

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

@benoitf, do you think we need to log that? For example, an output of user's command?

maybe not I was just trying to see which error message could be helpful to debug/understand what was the issue

@benoitf thank you, sometimes logged information isn't enough, but the problem here is the reason why it happens.

agree with Florent - toString here has no sense

I think it is still usable, because we will know consumer implementation

garagatyi · 2016-09-26T15:20:55Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/CompositeLineConsumer.java


    public CompositeLineConsumer(LineConsumer... lineConsumers) {
-        this.lineConsumers = lineConsumers;
+        this.lineConsumers = new ArrayList<>(lineConsumers.length);


Arrays.asList();

garagatyi · 2016-09-26T15:22:43Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/CompositeLineConsumer.java

 */
 public class CompositeLineConsumer implements LineConsumer {
    private static final Logger LOG = LoggerFactory.getLogger(CompositeLineConsumer.class);

-    private final LineConsumer[] lineConsumers;
+    private final List<LineConsumer> lineConsumers;
+    private       boolean            isClosed;


Will it work in multi-threaded environment?

garagatyi · 2016-09-26T15:25:02Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/CompositeLineConsumer.java

+                try {
+                    lineConsumer.writeLine(line);
+                } catch (ConsumerAlreadyClosedException | ClosedByInterruptException e) {
+                    lineConsumers.remove(lineConsumer); // consumer is already closed, so we cannot write into it any more


Is it an appropriate way to remove from source while you are iterating through it?

I would say a test is required with isClosed and writeLine throwing exceptions

@mmorhun Please address my comment

Fixed by using CopyOnWriteArrayList

garagatyi · 2016-09-26T15:30:43Z

...ore-api-core/src/main/java/org/eclipse/che/api/core/util/ConsumerAlreadyClosedException.java

+import java.io.IOException;
+
+/**
+ * @author Mykola Morhun


Signed-off-by: Mykola Morhun <[email protected]>

codenvy-ci · 2016-09-27T16:53:32Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/535/

Signed-off-by: Mykola Morhun <[email protected]>

codenvy-ci · 2016-09-28T06:51:01Z

Build # 539 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/539/ to view the results.

garagatyi · 2016-09-28T13:13:55Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/CompositeLineConsumer.java

-            } catch (IOException e) {
-                LOG.error(String.format("An error occurred while closing the line consumer %s", lineConsumer), e);
+        if (!isClosed) {
+            isClosed = true;


These operations are not atomic. Consider usage of AtomicBoolean

garagatyi · 2016-09-28T13:18:23Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/CompositeLineConsumer.java

+                try {
+                    lineConsumer.close();
+                } catch (IOException e) {
+                    LOG.error(String.format("An error occurred while closing the line consumer %s", lineConsumer), e);


I think that toString in line consumers returns pointer to memory area. Do we need that in error message?

Yes, because it also returns name of implementation in which problem occurs

garagatyi · 2016-09-28T13:18:48Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/CompositeLineConsumer.java

+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;

 /**
 * @author andrew00x


Please add javadocs

garagatyi · 2016-09-28T13:19:47Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/CompositeLineConsumer.java


    public CompositeLineConsumer(LineConsumer... lineConsumers) {
-        this.lineConsumers = lineConsumers;
+        this.lineConsumers = new CopyOnWriteArrayList<>(lineConsumers);
+        this.isClosed = false;
    }

    @Override
    public void close() throws IOException {


Please add javadoc that explains behavior of this implementation of consumer for writeLine and close methods

garagatyi · 2016-09-28T13:21:12Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/CompositeLineConsumer.java

+                try {
+                    lineConsumer.writeLine(line);
+                } catch (ConsumerAlreadyClosedException | ClosedByInterruptException e) {
+                    lineConsumers.remove(lineConsumer); // consumer is already closed, so we cannot write into it any more


@mmorhun Please address my comment

garagatyi · 2016-09-28T13:23:06Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/CompositeLineConsumer.java

+            for (LineConsumer lineConsumer : lineConsumers) {
+                try {
+                    lineConsumer.writeLine(line);
+                } catch (ConsumerAlreadyClosedException | ClosedByInterruptException e) {


If ClosedByInterruptException is thrown this class should not continue work

Why? Only one subconsumer is closed. What is the reason to close others?

garagatyi · 2016-09-28T13:23:56Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/CompositeLineConsumer.java

+                        isClosed = true;
+                    }
+                } catch (IOException e) {
+                    LOG.error(String.format("An error occurred while writing line to the line consumer %s", lineConsumer), e);


agree with Florent - toString here has no sense

garagatyi · 2016-09-28T13:25:10Z

...plugin-docker-client/src/main/java/org/eclipse/che/plugin/docker/client/ProgressMonitor.java

@@ -19,7 +20,7 @@
 * @author Alexander Garagatyi
 */
 public interface ProgressMonitor {
-    void updateProgress(ProgressStatus currentProgressStatus);


Throwing this exception here doesn't make sense because this class doesn't declare any dependency on line consumer

garagatyi · 2016-09-28T13:41:47Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/FileLineConsumer.java

@@ -34,11 +34,18 @@ public File getFile() {

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


please check closing status here

codenvy-ci · 2016-09-29T14:43:01Z

Build # 568 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/568/ to view the results.

Signed-off-by: Mykola Morhun <[email protected]>

codenvy-ci · 2016-10-04T16:38:16Z

Build # 612 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/612/ to view the results.

codenvy-ci · 2016-10-18T13:11:05Z

Build # 732 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/732/ to view the results.

Signed-off-by: Mykola Morhun <[email protected]>

garagatyi · 2016-10-31T15:03:25Z

...rc/main/java/org/eclipse/che/api/core/util/lineconsumer/ConcurrentCompositeLineConsumer.java

+     */
+    @Override
+    public void close() {
+        if (isOpen) {


Is this thread-safe?

thx, I forgot volatile

garagatyi · 2016-10-31T15:05:17Z

...rc/main/java/org/eclipse/che/api/core/util/lineconsumer/ConcurrentCompositeLineConsumer.java

+     */
+    @Override
+    public void writeLine(String line) {
+        if (isOpen && lock.readLock().tryLock()) {


Is this thread-safe?

garagatyi · 2016-10-31T15:10:35Z

...ore/src/main/java/org/eclipse/che/api/core/util/lineconsumer/ConcurrentFileLineConsumer.java

+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+/**
+ * @author andrew00x


garagatyi · 2016-10-31T15:11:11Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/CompositeLineConsumer.java


 /**
+ * This line consumer consists of several consumers and copies each consumed line into all subconsumers.
+ * Is used when lines should be written into two or more consumers.


Add a note that it is not thread-safe

garagatyi · 2016-10-31T15:11:40Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/FileLineConsumer.java

@@ -18,14 +20,20 @@

 /**
 * @author andrew00x


Javadocs? And add a note that it is not thread-safe

garagatyi · 2016-10-31T15:30:28Z

core/che-core-api-core/src/test/java/org/eclipse/che/api/core/util/FileLineConsumerTest.java

+    public void shouldBeAbleToWriteIntoFile() throws Exception {
+        // given
+        final String message = "Test line";
+        injectWriterMock(fileLineConsumer, writer);


Is it better to do that in single place?

garagatyi · 2016-10-31T15:32:21Z

...est/java/org/eclipse/che/api/core/util/lineconsumer/ConcurrentCompositeLineConsumerTest.java

+        concurrentCompositeLineConsumer.writeLine(message);
+
+        // then
+        for (LineConsumer subConsumer : subConsumers) {


The same comments to the whole class as to the non multi-threaded version.

garagatyi · 2016-10-31T15:32:44Z

...est/java/org/eclipse/che/api/core/util/lineconsumer/ConcurrentCompositeLineConsumerTest.java

+            verify(subConsumer, never()).writeLine(eq(message));
+        }
+    }
+


I don't see any multi-threaded tests

garagatyi · 2016-10-31T15:33:27Z

...src/test/java/org/eclipse/che/api/core/util/lineconsumer/ConcurrentFileLineConsumerTest.java

+        writerField.set(concurrentFileLineConsumer, writerMock);
+    }
+
+}


See comments above. I don't see multi-threaded tests

garagatyi · 2016-10-31T15:33:41Z

...plugin-docker-client/src/main/java/org/eclipse/che/plugin/docker/client/ProgressMonitor.java

@@ -12,6 +12,8 @@

 import org.eclipse.che.plugin.docker.client.json.ProgressStatus;

+import java.io.IOException;


Please clean up this class

codenvy-ci · 2016-10-31T16:12:32Z

Build # 860 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/860/ to view the results.

Signed-off-by: Mykola Morhun <[email protected]>

codenvy-ci · 2016-11-01T10:36:36Z

Build # 871 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/871/ to view the results.

Signed-off-by: Mykola Morhun <[email protected]>

codenvy-ci · 2016-11-01T18:45:53Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/885/

garagatyi · 2016-11-02T08:44:54Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/FileLineConsumer.java

+ * Consumes logs and writes them into file.
+ * <br/>
+ * This class is not thread safe.
+ * Also see multithreaded implementation {@link org.eclipse.che.api.core.util.lineconsumer.ConcurrentFileLineConsumer}


Links to other classes can be formatted with @see annotation

garagatyi · 2016-11-02T08:44:57Z

core/che-core-api-core/src/main/java/org/eclipse/che/api/core/util/CompositeLineConsumer.java

@@ -22,6 +22,9 @@
 /**
 * This line consumer consists of several consumers and copies each consumed line into all subconsumers.
 * Is used when lines should be written into two or more consumers.
+ * <br/>
+ * This class is not thread safe.
+ * Also see multithreaded implementation {@link org.eclipse.che.api.core.util.lineconsumer.ConcurrentCompositeLineConsumer}


Links to other classes can be formatted with @see annotation

garagatyi

Nice job!
Please consider suggested improvements

garagatyi · 2016-11-02T08:56:00Z

...che-core-api-core/src/test/java/org/eclipse/che/api/core/util/CompositeLineConsumerTest.java

            verify(subConsumer, never()).writeLine(eq(message));
        }
    }

+    private LineConsumer[] appendTo(LineConsumer[] base, LineConsumer... toAppend ) {
+        List<LineConsumer> allElements = new ArrayList<>(Arrays.asList(base));


I think that you can add base element in the head of list after creation of a list from other elements instead

garagatyi · 2016-11-02T09:26:41Z

...est/java/org/eclipse/che/api/core/util/lineconsumer/ConcurrentCompositeLineConsumerTest.java

+    }
+
+    private LineConsumer[] appendTo(LineConsumer[] base, LineConsumer... toAppend ) {
+        List<LineConsumer> allElements = new ArrayList<>(Arrays.asList(base));


See comment above

garagatyi · 2016-11-02T09:33:02Z

...est/java/org/eclipse/che/api/core/util/lineconsumer/ConcurrentCompositeLineConsumerTest.java

+    }
+
+    @Test
+    public void shouldIgnoreWriteToSubConsumersWhenLockForCloseIsLocked() throws Exception {


afterCloseWasCalled?

garagatyi · 2016-11-02T09:36:35Z

...est/java/org/eclipse/che/api/core/util/lineconsumer/ConcurrentCompositeLineConsumerTest.java

+        executor.execute(concurrentCompositeLineConsumer::close);
+
+        waitingAnswer1.completeAnswer();
+        waitingAnswer2.completeAnswer();


Please ensure that close methods weren't called before all writes are finished

garagatyi · 2016-11-02T09:37:34Z

...est/java/org/eclipse/che/api/core/util/lineconsumer/ConcurrentCompositeLineConsumerTest.java

+
+        for (LineConsumer consumer : subConsumers) {
+            verify(consumer).writeLine(eq(message1));
+            verify(consumer).writeLine(eq(message2));


what about verifying that close methods of internal consumers were called?

garagatyi · 2016-11-02T09:41:10Z

...src/test/java/org/eclipse/che/api/core/util/lineconsumer/ConcurrentFileLineConsumerTest.java

+        WaitingAnswer<Void> waitingAnswer = new WaitingAnswer<>();
+        doAnswer(waitingAnswer).when(writer).close();
+
+        executor.execute(this::closeConsumer);


I'm not sure that it is OK to ignore possible exception here

An exception never be thrown here because Writer is a mock.

garagatyi · 2016-11-02T09:41:36Z

...src/test/java/org/eclipse/che/api/core/util/lineconsumer/ConcurrentFileLineConsumerTest.java

+        waitingAnswer.waitAnswerCall(1, TimeUnit.SECONDS);
+
+        // when
+        writeIntoConsumer("Test line");


I'm not sure that it is OK to ignore possible exception here

I think, that it is OK. IMO, good test shouldn't know about an implementation, but be able to verify correct work of functionality. Here we shouldn't care about implementation behaviour in the test case (do nothing, log warning/error or throw an exception). All what we need to know that method which prints message in subconsumer wasn't invoked.

garagatyi · 2016-11-02T09:43:26Z

...src/test/java/org/eclipse/che/api/core/util/lineconsumer/ConcurrentFileLineConsumerTest.java

        concurrentFileLineConsumer = new ConcurrentFileLineConsumer(file);
+        injectWriterMock(concurrentFileLineConsumer, writer);
+
+        executor = Executors.newFixedThreadPool(5);


Do you really use 5 threadds here?

garagatyi · 2016-11-02T09:44:14Z

...est/java/org/eclipse/che/api/core/util/lineconsumer/ConcurrentCompositeLineConsumerTest.java

-        for (LineConsumer lineConsumer : subConsumers) {
-            doNothing().when(lineConsumer).writeLine(anyString());
-        }
+        executor = Executors.newFixedThreadPool(5);


Do you really use 5 threads here?

Signed-off-by: Mykola Morhun <[email protected]>

codenvy-ci · 2016-11-03T14:38:49Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/914/

Signed-off-by: Mykola Morhun <[email protected]>

codenvy-ci · 2016-11-04T09:26:03Z

Build # 921 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/921/ to view the results.

# Conflicts: # wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/environment/server/CheEnvironmentEngine.java

codenvy-ci · 2016-11-14T13:56:43Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1007/

…pse-che#2549)

benoitf reviewed Sep 23, 2016

View reviewed changes

garagatyi suggested changes Sep 26, 2016

View reviewed changes

CODENVY-587: Handle closed consumers in composite line comsumer

54a5b81

Signed-off-by: Mykola Morhun <[email protected]>

mmorhun force-pushed the CODENVY-587 branch from 2bcbebc to 54a5b81 Compare September 27, 2016 13:38

CODENVY-587: Handle closed consumers in composite line comsumer

5466e88

Signed-off-by: Mykola Morhun <[email protected]>

CODENVY-587: Handle closed consumers in composite line comsumer

65635ba

Signed-off-by: Mykola Morhun <[email protected]>

garagatyi suggested changes Sep 28, 2016

View reviewed changes

Merge branch 'master' into CODENVY-587

32e5728

Mykola Morhun added 2 commits October 4, 2016 17:24

Merge branch 'master' into CODENVY-587

384ae6f

CODENVY-587: Handle closed consumers in composite line comsumer

8d0e6d1

Signed-off-by: Mykola Morhun <[email protected]>

Merge branch 'master' into CODENVY-587

d561cdc

Mykola Morhun added 2 commits October 28, 2016 16:24

Merge branch 'master' into CODENVY-587

c52f6cb

Add concurrent versions of consumers

cce75a4

Signed-off-by: Mykola Morhun <[email protected]>

garagatyi suggested changes Oct 31, 2016

View reviewed changes

CODENVY-587: Clean tests and comments

b1c1f0b

Signed-off-by: Mykola Morhun <[email protected]>

Mykola Morhun added 3 commits November 1, 2016 18:13

Add multithreaded test

615be4e

Signed-off-by: Mykola Morhun <[email protected]>

Merge branch 'master' into CODENVY-587

2d2817b

Add license headers

c37ce05

Signed-off-by: Mykola Morhun <[email protected]>

garagatyi reviewed Nov 2, 2016

View reviewed changes

garagatyi approved these changes Nov 2, 2016

View reviewed changes

CODENVY-587: Correct some tests

73e86a0

Signed-off-by: Mykola Morhun <[email protected]>

CODENVY-587: Add last verification in test

37589f6

Signed-off-by: Mykola Morhun <[email protected]>

Merge branch 'master' into CODENVY-587

655c7d5

# Conflicts: # wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/environment/server/CheEnvironmentEngine.java

skabashnyuk approved these changes Nov 15, 2016

View reviewed changes

mmorhun added this to the 5.0.0-M8 milestone Nov 15, 2016

mmorhun added the team/enterprise label Nov 15, 2016

mmorhun merged commit 7f2875e into master Nov 15, 2016

mmorhun deleted the CODENVY-587 branch November 15, 2016 13:00

JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017

CODENVY-587: Handle closed consumers in composite line comsumer (ecli…

2b1c86e

…pse-che#2549)

		@@ -12,6 +12,8 @@

		import org.eclipse.che.plugin.docker.client.json.ProgressStatus;

		import java.io.IOException;

CODENVY-587: Handle closed consumers in composite line comsumer #2549

CODENVY-587: Handle closed consumers in composite line comsumer #2549

Conversation

mmorhun commented Sep 22, 2016 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Previous behavior

New behavior

mmorhun commented Sep 22, 2016

codenvy-ci commented Sep 22, 2016

benoitf Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Sep 27, 2016

codenvy-ci commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Sep 29, 2016

codenvy-ci commented Oct 4, 2016

codenvy-ci commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Oct 31, 2016

codenvy-ci commented Nov 1, 2016

codenvy-ci commented Nov 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garagatyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Nov 3, 2016

codenvy-ci commented Nov 4, 2016

codenvy-ci commented Nov 14, 2016

mmorhun commented Sep 22, 2016 •

edited

Loading

benoitf Sep 23, 2016 •

edited

Loading