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 4 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 @@ -17,6 +17,8 @@

import com.sun.codemodel.CodeWriter;
import com.sun.codemodel.JPackage;
import org.androidtransfuse.util.apache.commons.WriterOutputStream;


import javax.annotation.processing.Filer;
import javax.inject.Inject;
Expand All @@ -40,18 +42,16 @@ 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();
WriterOutputStream os = new WriterOutputStream(resource.openWriter(),"UTF-8");
openStreams.add(os);

return os;
}


@Override
public void close() throws IOException {
for (OutputStream openStream : openStreams) {
openStream.flush();
// 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 @@ -18,6 +18,7 @@
import com.sun.codemodel.CodeWriter;
import com.sun.codemodel.JPackage;
import org.androidtransfuse.adapter.PackageClass;
import org.androidtransfuse.util.apache.commons.WriterOutputStream;

import javax.annotation.processing.Filer;
import javax.inject.Inject;
Expand Down Expand Up @@ -49,10 +50,8 @@ 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 = new WriterOutputStream(sourceFile.openWriter(), "UTF-8");
Copy link
Owner

Choose a reason for hiding this comment

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

Pass in StandardCharsets.UTF_8 instead of the string?

Copy link
Author

Choose a reason for hiding this comment

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

ah yes, i just replace it

openStreams.add(os);

return os;
}

Expand All @@ -63,7 +62,7 @@ private String toQualifiedClassName(JPackage pkg, String fileName) {
@Override
public void close() throws IOException {
for (OutputStream openStream : openStreams) {
openStream.flush();
// openStream.flush();
Copy link
Owner

Choose a reason for hiding this comment

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

Remove

Copy link
Author

Choose a reason for hiding this comment

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

done

openStream.close();
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
package org.androidtransfuse.util.apache.commons;
Copy link
Owner

Choose a reason for hiding this comment

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

I'd stick this in org.androidtransfuse.gen next to FilerSourceCodeWriter

Copy link
Author

Choose a reason for hiding this comment

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

done, just moved next to FilerSourceCodeWriter



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 static final int DEFAULT_BUFFER_SIZE = 1024;
private final Writer writer;
private final CharsetDecoder decoder;
private final boolean writeImmediately;
private final ByteBuffer decoderIn;
private final CharBuffer decoderOut;

public WriterOutputStream(Writer writer, CharsetDecoder decoder) {
this(writer, (CharsetDecoder)decoder, 1024, false);
}

public WriterOutputStream(Writer writer, CharsetDecoder decoder, int bufferSize, boolean writeImmediately) {
this.decoderIn = ByteBuffer.allocate(128);
checkIbmJdkWithBrokenUTF16(decoder.charset());
this.writer = writer;
this.decoder = decoder;
this.writeImmediately = writeImmediately;
this.decoderOut = CharBuffer.allocate(bufferSize);
}

public WriterOutputStream(Writer writer, Charset charset, int bufferSize, boolean writeImmediately) {
this(writer, charset.newDecoder().onMalformedInput(CodingErrorAction.REPLACE).onUnmappableCharacter(CodingErrorAction.REPLACE).replaceWith("?"), bufferSize, writeImmediately);
}

public WriterOutputStream(Writer writer, Charset charset) {
this(writer, (Charset)charset, 1024, false);
}

public WriterOutputStream(Writer writer, String charsetName, int bufferSize, boolean writeImmediately) {
this(writer, Charset.forName(charsetName), bufferSize, writeImmediately);
}

public WriterOutputStream(Writer writer, String charsetName) {
this(writer, (String)charsetName, 1024, false);
}

/** @deprecated */
@Deprecated
public WriterOutputStream(Writer writer) {
this(writer, (Charset)Charset.defaultCharset(), 1024, false);
}

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();
}

}

private static void checkIbmJdkWithBrokenUTF16(Charset charset) {
if ("UTF-16".equals(charset.name())) {
String TEST_STRING_2 = "vés";
byte[] bytes = "vés".getBytes(charset);
CharsetDecoder charsetDecoder2 = charset.newDecoder();
ByteBuffer bb2 = ByteBuffer.allocate(16);
CharBuffer cb2 = CharBuffer.allocate("vés".length());
int len = bytes.length;

for(int i = 0; i < len; ++i) {
bb2.put(bytes[i]);
bb2.flip();

try {
charsetDecoder2.decode(bb2, cb2, i == len - 1);
} catch (IllegalArgumentException var9) {
throw new UnsupportedOperationException("UTF-16 requested when runninng on an IBM JDK with broken UTF-16 support. Please find a JDK that supports UTF-16 if you intend to use UF-16 with WriterOutputStream");
}

bb2.compact();
}

cb2.rewind();
if (!"vés".equals(cb2.toString())) {
throw new UnsupportedOperationException("UTF-16 requested when runninng on an IBM JDK with broken UTF-16 support. Please find a JDK that supports UTF-16 if you intend to use UF-16 with WriterOutputStream");
}
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Go ahead and get rid of anything not used. Since we're using utf-8, the checkIbmJdkWithBrokenUTF16 will never execute, and most of the constructors can be combined into one.

Copy link
Author

Choose a reason for hiding this comment

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

done, i just removed it and combine the constructor.

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 @@ -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();
}
}
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.

}
}