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.

Sign up for GitHub

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

Jump to bottom

Replace outputstream with writeroutputstream #234

Merged
merged 7 commits into from
Jul 17, 2021
Merged

Replace outputstream with writeroutputstream #234

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fbb1b5f
repalce output stream with writeroutputstream
Nov 2, 2020
5fdd78b
remove unused variable
Nov 2, 2020
59b671c
Copy WriterOutputStream locally and add charset
Nov 5, 2020
8355efd
add test scope on tranfuse/pom.xml
Nov 5, 2020
2e2f68d
Change to StandartCharsets, remove comments, update common-io on tran…
Nov 5, 2020
b7b9748
fix test and move writer to gen folder
Nov 6, 2020
4e6e310
change standardCharsets to charset.forname
Nov 7, 2020
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
4 changes: 2 additions & 2 deletions transfuse-core/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@
<dependency>
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
<version>2.4</version>
<scope>test</scope>
doniwinata0309 marked this conversation as resolved.
Show resolved Hide resolved
<version>2.5</version>

</dependency>
<dependency>
<groupId>org.mockito</groupId>
Expand Down
11 changes: 3 additions & 8 deletions transfuse-core/src/main/java/org/androidtransfuse/gen/FilerResourceWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import com.sun.codemodel.CodeWriter;
import com.sun.codemodel.JPackage;
import org.apache.commons.io.output.WriterOutputStream;

import javax.annotation.processing.Filer;
import javax.inject.Inject;
Expand All @@ -30,7 +31,6 @@
public class FilerResourceWriter extends CodeWriter {

private final Filer filer;
private final Collection<OutputStream> openStreams = new HashSet<OutputStream>();

@Inject
public FilerResourceWriter(Filer filer) {
Expand All @@ -41,18 +41,13 @@ public FilerResourceWriter(Filer filer) {
public OutputStream openBinary(JPackage pkg, String fileName) throws IOException {
FileObject resource = filer.createResource(StandardLocation.SOURCE_OUTPUT, pkg.name(), fileName);

OutputStream os = resource.openOutputStream();
openStreams.add(os);

OutputStream os = new WriterOutputStream(resource.openWriter());
Copy link
Owner

Choose a reason for hiding this comment

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

This constructor is depreciated, we should define a charset.

Copy link
Author

Choose a reason for hiding this comment

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

I changed it to
OutputStream os = new WriterOutputStream(sourceFile.openWriter(), "UTF-8");

is it okay to use UTF-8 ?

return os;
}


@Override
public void close() throws IOException {
for (OutputStream openStream : openStreams) {
openStream.flush();
openStream.close();
}
Copy link
Owner

Choose a reason for hiding this comment

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

Why not keep these calls? It seems like they would be necessary because we'll be dealing with a buffer with WriterOutputStream.

Copy link
Author

Choose a reason for hiding this comment

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

it is causing this error

Caused by: java.io.IOException: Stream closed
    at sun.nio.cs.StreamEncoder.ensureOpen (StreamEncoder.java:45)
    at sun.nio.cs.StreamEncoder.flush (StreamEncoder.java:140)
    at java.io.OutputStreamWriter.flush (OutputStreamWriter.java:229)
    at java.io.FilterWriter.flush (FilterWriter.java:100)
    at org.androidtransfuse.util.apache.commons.WriterOutputStream.flush (WriterOutputStream.java:83)
    at org.androidtransfuse.gen.FilerSourceCodeWriter.close (FilerSourceCodeWriter.java:65)
    at com.sun.codemodel.JCodeModel.build (JCodeModel.java:312)

Seems it already closed by the WriterOutputStream

    public void close() throws IOException {
        this.processInput(true);
        this.flushOutput();
        this.writer.close();
    }

Do you have other clue why it causing error ?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, I bet codemodel is already closing the output stream and since it's closed flush throws an error. You can outright get rid of the flush call instead of just commenting it out.

Copy link
Author

@doniwinata0309 doniwinata0309 Nov 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

yes it seems already flush and also closed from the log, also looking from the flamegraph.


}
}
10 changes: 3 additions & 7 deletions transfuse-core/src/main/java/org/androidtransfuse/gen/FilerSourceCodeWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.io.OutputStream;
import java.util.Collection;
import java.util.HashSet;
import org.apache.commons.io.output.WriterOutputStream;

/**
* Adapter class to allow codemodel to write its output source and source files to the Java Annotation Processor Filer
Expand All @@ -36,7 +37,6 @@ public class FilerSourceCodeWriter extends CodeWriter {

private final Filer filer;
private final Originating originating;
private final Collection<OutputStream> openStreams = new HashSet<OutputStream>();

@Inject
public FilerSourceCodeWriter(Filer filer, Originating originating) {
Expand All @@ -50,9 +50,8 @@ public OutputStream openBinary(JPackage jPackage, String fileName) throws IOExce
String qualified = toQualifiedClassName(jPackage, fileName);
JavaFileObject sourceFile = filer.createSourceFile(qualified, originating.getOriginatingElements(qualified));

OutputStream os = sourceFile.openOutputStream();
openStreams.add(os);

OutputStream os = new WriterOutputStream(sourceFile.openWriter());
return os;
}

Expand All @@ -62,9 +61,6 @@ private String toQualifiedClassName(JPackage pkg, String fileName) {

@Override
public void close() throws IOException {
for (OutputStream openStream : openStreams) {
openStream.flush();
openStream.close();
}

}
}
4 changes: 2 additions & 2 deletions transfuse/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@
<dependency>
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
<version>2.4</version>
<scope>test</scope>
<version>2.5</version>
Copy link
Owner

Choose a reason for hiding this comment

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

👍... there are a few more commons-io used in testing, could you upgrade those as well in this pass?

Copy link
Author

Choose a reason for hiding this comment

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

done


</dependency>
<dependency>
<groupId>org.mockito</groupId>
Expand Down
6 changes: 1 addition & 5 deletions transfuse/src/test/java/org/androidtransfuse/gen/FilerResourceWriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package org.androidtransfuse.gen;

import com.sun.codemodel.JCodeModel;
import org.apache.commons.io.output.WriterOutputStream;
import org.junit.Before;
import org.junit.Test;

Expand Down Expand Up @@ -58,10 +59,5 @@ public void testCreateResource() throws IOException {
when(mockFiler.createResource(StandardLocation.SOURCE_OUTPUT, TEST_PACKAGE, TEST_FILENAME)).thenReturn(mockFile);
when(mockFile.openOutputStream()).thenReturn(mockOutputStream);

assertEquals(mockOutputStream, resourceWriter.openBinary(codeModel._package(TEST_PACKAGE), TEST_FILENAME));
doniwinata0309 marked this conversation as resolved.
Show resolved Hide resolved

resourceWriter.close();
verify(mockOutputStream).flush();
verify(mockOutputStream).close();
}
}
7 changes: 0 additions & 7 deletions transfuse/src/test/java/org/androidtransfuse/gen/FilerSourceCodeWriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,7 @@ public void setUp() throws Exception {

@Test
public void testCreateSourceFile() throws IOException {

when(mockFiler.createSourceFile(TEST_PACKAGE + "." + TEST_CLASS)).thenReturn(mockFile);
when(mockFile.openOutputStream()).thenReturn(mockOutputStream);

assertEquals(mockOutputStream, codeWriter.openBinary(codeModel._package(TEST_PACKAGE), TEST_CLASS));

codeWriter.close();
verify(mockOutputStream).flush();
verify(mockOutputStream).close();
Copy link
Owner

Choose a reason for hiding this comment

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

In general, I'd really like to keep these tests.

Copy link
Author

Choose a reason for hiding this comment

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

now it seems this test not doing anything right ? do you have idea what we should test, or should we remove it ?

Copy link
Owner

Choose a reason for hiding this comment

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

I would breing everything back except for verify(mockOutputStream).flush();

Copy link
Author

Choose a reason for hiding this comment

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

I just modify the test, and add spy there when creating codeWriter object. Also, i put a new method so we can mock this part
doReturn(mockOutputStream).when(codeWriter).getWriterOutputStream(mockWriter);
please let me know if you have better approach on this.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, great, I think that works.

}
}