8247918: Clarify Reader.skip behavior for end of stream

Reviewed-by: rriggs, naoto

src/java.base/share/classes/java/io/BufferedReader.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1996, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -397,14 +397,7 @@ public class BufferedReader extends Reader {
     }
 
     /**
-     * Skips characters.
+     * {@inheritDoc}
-     *
-     * @param  n  The number of characters to skip
-     *
-     * @return    The number of characters actually skipped
-     *
-     * @throws     IllegalArgumentException  If {@code n} is negative.
-     * @throws     IOException  If an I/O error occurs
      */
     public long skip(long n) throws IOException {
         if (n < 0L) {

src/java.base/share/classes/java/io/CharArrayReader.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1996, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -146,16 +146,19 @@ public class CharArrayReader extends Reader {
     }
 
     /**
-     * Skips characters.  Returns the number of characters that were skipped.
+     * Skips characters. If the stream is already at its end before this method
+     * is invoked, then no characters are skipped and zero is returned.
      *
      * <p>The {@code n} parameter may be negative, even though the
      * {@code skip} method of the {@link Reader} superclass throws
      * an exception in this case. If {@code n} is negative, then
      * this method does nothing and returns {@code 0}.
      *
-     * @param      n The number of characters to skip
+     * @param n {@inheritDoc}
-     * @return     The number of characters actually skipped
+     *
-     * @throws     IOException If the stream is closed, or an I/O error occurs
+     * @return {@inheritDoc}
+     *
+     * @throws IOException {@inheritDoc}
      */
     public long skip(long n) throws IOException {
         synchronized (lock) {

src/java.base/share/classes/java/io/FilterReader.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1996, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -76,9 +76,11 @@ public abstract class FilterReader extends Reader {
     }
 
     /**
-     * Skips characters.
+     * {@inheritDoc}
      *
-     * @throws     IOException  If an I/O error occurs
+     * @throws     IllegalArgumentException  If {@code n} is negative and the
+     *             contained {@code Reader}'s {@code skip} method throws an
+     *             IllegalArgumentException for a negative parameter
      */
     public long skip(long n) throws IOException {
         return in.skip(n);

src/java.base/share/classes/java/io/LineNumberReader.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1996, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -267,18 +267,7 @@ public class LineNumberReader extends BufferedReader {
     private char skipBuffer[] = null;
 
     /**
-     * Skip characters.
+     * {@inheritDoc}
-     *
-     * @param  n
-     *         The number of characters to skip
-     *
-     * @return  The number of characters actually skipped
-     *
-     * @throws  IOException
-     *          If an I/O error occurs
-     *
-     * @throws  IllegalArgumentException
-     *          If {@code n} is negative
      */
     public long skip(long n) throws IOException {
         if (n < 0)

src/java.base/share/classes/java/io/PushbackReader.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1996, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -254,15 +254,7 @@ public class PushbackReader extends FilterReader {
     }
 
     /**
-     * Skips characters.  This method will block until some characters are
+     * {@inheritDoc}
-     * available, an I/O error occurs, or the end of the stream is reached.
-     *
-     * @param     n  The number of characters to skip
-     *
-     * @return    The number of characters actually skipped
-     *
-     * @throws    IllegalArgumentException  If {@code n} is negative.
-     * @throws    IOException  If an I/O error occurs
      */
     public long skip(long n) throws IOException {
         if (n < 0L)

src/java.base/share/classes/java/io/Reader.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1996, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -269,6 +269,8 @@ public abstract class Reader implements Readable, Closeable {
     /**
      * Skips characters.  This method will block until some characters are
      * available, an I/O error occurs, or the end of the stream is reached.
+     * If the stream is already at its end before this method is invoked,
+     * then no characters are skipped and zero is returned.
      *
      * @param  n  The number of characters to skip
      *

src/java.base/share/classes/java/io/StringReader.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1996, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -105,31 +105,35 @@ public class StringReader extends Reader {
     }
 
     /**
-     * Skips the specified number of characters in the stream. Returns
+     * Skips characters. If the stream is already at its end before this method
-     * the number of characters that were skipped.
+     * is invoked, then no characters are skipped and zero is returned.
      *
-     * <p>The {@code ns} parameter may be negative, even though the
+     * <p>The {@code n} parameter may be negative, even though the
      * {@code skip} method of the {@link Reader} superclass throws
-     * an exception in this case. Negative values of {@code ns} cause the
+     * an exception in this case. Negative values of {@code n} cause the
      * stream to skip backwards. Negative return values indicate a skip
      * backwards. It is not possible to skip backwards past the beginning of
      * the string.
      *
      * <p>If the entire string has been read or skipped, then this method has
-     * no effect and always returns 0.
+     * no effect and always returns {@code 0}.
      *
-     * @throws     IOException  If an I/O error occurs
+     * @param n {@inheritDoc}
+     *
+     * @return {@inheritDoc}
+     *
+     * @throws IOException {@inheritDoc}
      */
-    public long skip(long ns) throws IOException {
+    public long skip(long n) throws IOException {
         synchronized (lock) {
             ensureOpen();
             if (next >= length)
                 return 0;
             // Bound skip by beginning and end of the source
-            long n = Math.min(length - next, ns);
+            long r = Math.min(length - next, n);
-            n = Math.max(-next, n);
+            r = Math.max(-next, r);
-            next += n;
+            next += r;
-            return n;
+            return r;
         }
     }
 

test/jdk/java/io/Reader/Skip.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2010, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -22,29 +22,83 @@
  */
 
 /* @test
-   @bug 4134311
+ * @bug 4134311 8247918
-   @summary Test if skip works correctly
+ * @summary Test if skip works correctly
+ * @run testng Skip
 */
 
+import java.io.CharArrayReader;
+import java.io.File;
+import java.io.FileReader;
+import java.io.IOException;
+import java.io.LineNumberReader;
+import java.io.PushbackReader;
+import java.io.RandomAccessFile;
+import java.io.Reader;
+import java.io.StringReader;
 
-
+import org.testng.Assert;
-import java.io.*;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
 
 public class Skip {
-    public static void main(String argv[]) throws Exception {
+    private static String FILENAME =
-        File f = new File(System.getProperty("test.src", "."),
+        System.getProperty("test.src", ".") + File.separator + "SkipInput.txt";
-                          "SkipInput.txt");
+    private static File file = new File(FILENAME);
-        FileReader fr = new FileReader(f);
+
-        try {
+    @Test
+    public void skip() throws IOException {
+        try (FileReader fr = new FileReader(file)) {
             long nchars = 8200;
             long actual = fr.skip(nchars);
 
-            if (actual > nchars) {
+            Assert.assertFalse(actual > nchars,
-                throw new Exception
+                "Should skip " + nchars + ", but skipped " +actual+" chars");
-                    ("Should skip " + nchars + ", but skipped " +actual+" chars");
-            }
-        } finally {
-            fr.close();
         }
     }
+
+    @DataProvider(name = "readers")
+    public Object[][] getReaders() throws IOException {
+        return new Object[][] {
+            {new LineNumberReader(new FileReader(file))},
+            {new CharArrayReader(new char[] {27})},
+            {new PushbackReader(new FileReader(file))},
+            {new FileReader(file)},
+            {new StringReader(new String(new byte[] {(byte)42}))}
+        };
+    }
+
+    @Test(dataProvider = "readers")
+    public void eof(Reader r) throws IOException {
+         r.skip(Long.MAX_VALUE);
+         Assert.assertEquals(r.skip(1), 0);
+         Assert.assertEquals(r.read(), -1);
+    }
+
+    @DataProvider(name = "skipIAE")
+    public Object[][] getSkipIAEs() throws IOException {
+        return new Object[][] {
+            {new LineNumberReader(new FileReader(file))},
+            {new PushbackReader(new FileReader(file))},
+            {new FileReader(file)}
+        };
+    }
+
+    @Test(dataProvider = "skipIAE", expectedExceptions = IllegalArgumentException.class)
+    public void testThrowsIAE(Reader r) throws IOException {
+        r.skip(-1);
+    }
+
+    @DataProvider(name = "skipNoIAE")
+    public Object[][] getSkipNoIAEs() throws IOException {
+        return new Object[][] {
+            {new CharArrayReader(new char[] {27})},
+            {new StringReader(new String(new byte[] {(byte)42}))}
+        };
+    }
+
+    @Test(dataProvider = "skipNoIAE")
+    public void testNoIAE(Reader r) throws IOException {
+        r.skip(-1);
+    }
 }