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

Replace outputstream with writeroutputstream #234

Merged
merged 7 commits into from
Jul 17, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
3 changes: 2 additions & 1 deletion transfuse-core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,9 @@
<dependency>
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
<version>2.4</version>
<version>2.5</version>
<scope>test</scope>
doniwinata0309 marked this conversation as resolved.
Show resolved Hide resolved

</dependency>
<dependency>
<groupId>org.mockito</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import javax.tools.StandardLocation;
import java.io.IOException;
import java.io.OutputStream;
import java.io.Writer;
import java.nio.charset.Charset;
import java.util.Collection;
import java.util.HashSet;

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

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

return os;
}

public OutputStream getWriterOutputStream(Writer writer) {
return new WriterOutputStream(writer, Charset.forName("UTF-8"));
}

@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

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.

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import javax.tools.JavaFileObject;
import java.io.IOException;
import java.io.OutputStream;
import java.io.Writer;
import java.nio.charset.Charset;
import java.util.Collection;
import java.util.HashSet;

Expand All @@ -49,21 +51,23 @@ public OutputStream openBinary(JPackage jPackage, String fileName) throws IOExce
//generate a source file based on package and fileName
String qualified = toQualifiedClassName(jPackage, fileName);
JavaFileObject sourceFile = filer.createSourceFile(qualified, originating.getOriginatingElements(qualified));

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

return os;
}

public OutputStream getWriterOutputStream(Writer writer) {
return new WriterOutputStream(writer, Charset.forName("UTF-8"));
}

private String toQualifiedClassName(JPackage pkg, String fileName) {
return new PackageClass(pkg.name(), fileName).getFullyQualifiedName();
}

@Override
public void close() throws IOException {
for (OutputStream openStream : openStreams) {
openStream.flush();
openStream.close();
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package org.androidtransfuse.gen;


import java.io.IOException;
import java.io.OutputStream;
import java.io.Writer;
import java.nio.ByteBuffer;
import java.nio.CharBuffer;
import java.nio.charset.Charset;
import java.nio.charset.CharsetDecoder;
import java.nio.charset.CoderResult;
import java.nio.charset.CodingErrorAction;


public class WriterOutputStream extends OutputStream {

private final Writer writer;
private final CharsetDecoder decoder;
private final boolean writeImmediately;
private final ByteBuffer decoderIn;
private final CharBuffer decoderOut;

public WriterOutputStream(Writer writer, Charset charset) {
this.decoderIn = ByteBuffer.allocate(128);
this.writer = writer;
this.decoder = charset.newDecoder().onMalformedInput(CodingErrorAction.REPLACE).onUnmappableCharacter(CodingErrorAction.REPLACE).replaceWith("?");
this.writeImmediately = false;
this.decoderOut = CharBuffer.allocate(1024);
}

public void write(byte[] b, int off, int len) throws IOException {
while (len > 0) {
int c = Math.min(len, this.decoderIn.remaining());
this.decoderIn.put(b, off, c);
this.processInput(false);
len -= c;
off += c;
}

if (this.writeImmediately) {
this.flushOutput();
}
}

public void write(byte[] b) throws IOException {
this.write(b, 0, b.length);
}

public void write(int b) throws IOException {
this.write(new byte[]{(byte) b}, 0, 1);
}

public void flush() throws IOException {
this.flushOutput();
this.writer.flush();
}

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

private void processInput(boolean endOfInput) throws IOException {
this.decoderIn.flip();

while (true) {
CoderResult coderResult = this.decoder.decode(this.decoderIn, this.decoderOut, endOfInput);
if (!coderResult.isOverflow()) {
if (coderResult.isUnderflow()) {
this.decoderIn.compact();
return;
} else {
throw new IOException("Unexpected coder result");
}
}

this.flushOutput();
}
}

private void flushOutput() throws IOException {
if (this.decoderOut.position() > 0) {
this.writer.write(this.decoderOut.array(), 0, this.decoderOut.position());
this.decoderOut.rewind();
}
}
}
2 changes: 1 addition & 1 deletion transfuse-support/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@
<dependency>
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
<version>2.4</version>
<version>2.5</version>
<scope>test</scope>
</dependency>
</dependencies>
Expand Down
3 changes: 2 additions & 1 deletion transfuse/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,9 @@
<dependency>
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
<version>2.4</version>
<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

<scope>test</scope>

</dependency>
<dependency>
<groupId>org.mockito</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,15 @@
import javax.tools.StandardLocation;
import java.io.IOException;
import java.io.OutputStream;
import java.io.Writer;

import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.*;

/**
* @author John Ericksen
*/

public class FilerResourceWriterTest {

private static final String TEST_PACKAGE = "org.test";
Expand All @@ -40,28 +42,27 @@ public class FilerResourceWriterTest {
private Filer mockFiler;
private FileObject mockFile;
private OutputStream mockOutputStream;
private Writer mockWriter;
private JCodeModel codeModel;

@Before
public void setUp() throws Exception {
mockFiler = mock(Filer.class);
mockFile = mock(FileObject.class);
mockOutputStream = mock(OutputStream.class);
mockWriter = mock(Writer.class);

resourceWriter = new FilerResourceWriter(mockFiler);
resourceWriter = spy(new FilerResourceWriter(mockFiler));
codeModel = new JCodeModel();
}

@Test
public void testCreateResource() throws IOException {

when(mockFiler.createResource(StandardLocation.SOURCE_OUTPUT, TEST_PACKAGE, TEST_FILENAME)).thenReturn(mockFile);
when(mockFile.openOutputStream()).thenReturn(mockOutputStream);

doReturn(mockWriter).when(mockFile).openWriter();
doReturn(mockOutputStream).when(resourceWriter).getWriterOutputStream(mockWriter);
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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,19 @@
import org.junit.Before;
import org.junit.Test;


import javax.annotation.processing.Filer;
import javax.tools.JavaFileObject;
import java.io.IOException;
import java.io.OutputStream;

import java.io.Writer;
import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.*;


/**
* @author John Ericksen
*/

public class FilerSourceCodeWriterTest {

private static final String TEST_PACKAGE = "org.test";
Expand All @@ -39,28 +41,26 @@ public class FilerSourceCodeWriterTest {
private Filer mockFiler;
private JavaFileObject mockFile;
private OutputStream mockOutputStream;
private Writer mockWriter;
private JCodeModel codeModel;

@Before
public void setUp() throws Exception {
mockWriter = mock(Writer.class);
mockFiler = mock(Filer.class);
mockFile = mock(JavaFileObject.class);
mockOutputStream = mock(OutputStream.class);

codeWriter = new FilerSourceCodeWriter(mockFiler, new Originating());
codeWriter = spy(new FilerSourceCodeWriter(mockFiler, new Originating()));
codeModel = new JCodeModel();
}

@Test
public void testCreateSourceFile() throws IOException {

public void testCreateSourceFile() throws Exception {
when(mockFiler.createSourceFile(TEST_PACKAGE + "." + TEST_CLASS)).thenReturn(mockFile);
when(mockFile.openOutputStream()).thenReturn(mockOutputStream);

doReturn(mockWriter).when(mockFile).openWriter();
doReturn(mockOutputStream).when(codeWriter).getWriterOutputStream(mockWriter);
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.

}
}