Thanks Paul!

Webrev has been updated to include your test, and to fix that 
stackoverflowexception, triggered
by the recursive implementation of StreamEncoder.flushLeftoverChar. The method 
really does
not need to be recursive (an alternative is to push the "leftover" from the 
input buffer back to
the buffer, but it appears the proposed change is simple)

http://cr.openjdk.java.net/~sherman/8030179/webrev/

-Sherman

On 02/06/2015 02:03 AM, Paul Sandoz wrote:
On Feb 5, 2015, at 9:50 PM, Xueming Shen<xueming.s...@oracle.com>  wrote:

On 02/05/2015 12:47 PM, Paul Sandoz wrote:
On Feb 5, 2015, at 8:00 PM, Xueming Shen<xueming.s...@oracle.com>   wrote:

Hi,

Please help review the fix for #8030179

issue: https://bugs.openjdk.java.net/browse/JDK-8030179
webrev: http://cr.openjdk.java.net/~sherman/8030179/webrev

This is the regression bug introduced in jdk7 when trying to optimize the single
byte encoding loop, in which the "optimization" code inappropriately updates the
"sl" (source limit) value and triggers misbehavior of the sgp parser (in which 
it
mistakenly returns "underflow" when it sees a high surrogate but can't see the 
next
low surrogate, because the "sl" is changed...)

The fix looks good, but i would like to suggest some changes to the test.

We can use TestNG to avoid pulling in a bunch of infrastructure methods.

The test cases can be produced by a data provider and we can do a cross product 
with char sets and character input (for example i have managed to induced stack 
overflows by just passing sequences of high surrogates).

Using the data provider allows the test infrastrcture to report/crunch those 
tests individually rather than being reported via System.out.

I can send a patch to the test if that helps.
That will be appreciated :-)

See below. On my Mac this runs 504 tests, 4 to 6 of which fail (i dunno where 
the non-determinism comes from).

In the jtreg reported output you will see stuff like this:

   test StreamEncoderOut.test(x-windows-iso2022jp, HIGH_LOW : '\ud834\udd1e"): 
success

Unfortunately because a StackOverflowException is printed out jtreg truncates 
the middle part of the output (and the system property jtreg says one can use 
to increase that output does not appear to work) so it's tricky to work out  
what actually failed without explicitly catching the SOE and re-throwing as an 
assertion error.

(We should have a better way of printing out the stack trace of an SOE.)

So you could remove the Input.HIGH value in the test, commit, and log a another 
bug for the SOE, then add it back for the fix.

Paul.

/* @test
    @bug 8030179
    @summary test if the charset encoder deails with surrogate correctly
  * @run testng/othervm -esa StreamEncoderOut
  */

import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

import java.io.IOException;
import java.io.OutputStream;
import java.io.OutputStreamWriter;
import java.nio.charset.Charset;
import java.util.stream.Stream;

import static java.util.stream.Collectors.joining;

@Test
public class StreamEncoderOut {

     enum Input {
         HIGH("\ud834"),
         LOW("\udd1e"),
         HIGH_LOW("\ud834\udd1e");

         final String value;

         Input(String value) {
             this.value = value;
         }

         @Override
         public String toString() {
             return name() + " : \'" + value + "\"";
         }
     }

     @DataProvider(name = "CharsetAndString")
     // [Charset, Input]
     public static Object[][] makeStreamTestData() {
         // Cross product of supported charsets and inputs
         return Charset.availableCharsets().values().stream().
                 filter(Charset::canEncode).
                 flatMap(c ->  Stream.of(Input.values()).map(i ->  new 
Object[]{c, i})).
                 toArray(Object[][]::new);
     }

     private static String generate(String s, int n) {
         return Stream.generate(() ->  s).limit(n).collect(joining());
     }

     static final OutputStream DEV_NULL = new OutputStream() {
         @Override
         public void write(byte b[], int off, int len) throws IOException {}

         @Override
         public void write(int b) throws IOException {}
     };

     @Test(dataProvider = "CharsetAndString")
     public void test(Charset cs, Input input) throws IOException {
         OutputStreamWriter w = new OutputStreamWriter(DEV_NULL, cs);
         String t = generate(input.value, 8193);
         for (int i = 0; i<  10; i++) {
             w.append(t);
         }
     }
}

Reply via email to