You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "yiding-he (GitHub)" <gi...@apache.org> on 2019/02/01 12:14:48 UTC
[GitHub] [incubator-dubbo] yiding-he opened issue #3420: DO NOT USE
org.apache.dubbo.common.utils.IOUtils.write() for your own project
- [x] I have searched the [issues](https://github.com/apache/incubator-dubbo/issues) of this repository and believe that this is not a duplicate.
- [x] I have checked the [FAQ](https://github.com/apache/incubator-dubbo/blob/master/FAQ.md) of this repository and believe that this is not a duplicate.
### Environment
* Dubbo version: 2.7.1-SNAPSHOT
`org.apache.dubbo.common.utils.IOUtils` 这个类看起来像是一个公共类，但实际上里面有坑，不要在你自己的项目里面使用这个类！
### 问题描述
该类有一个 write() 方法，内容如下：
```
public static long write(InputStream is, OutputStream os, int bufferSize) throws IOException {
int read;
long total = 0;
byte[] buff = new byte[bufferSize];
while (is.available() > 0) { // !!!
read = is.read(buff, 0, buff.length);
if (read > 0) {
os.write(buff, 0, read);
total += read;
}
}
return total;
}
```
其中的 while 循环使用了 `InputStream.available()` 的返回值来作为结束判断条件，这个方法在网络环境当中是不能作为读取结束的依据的，因为当网络传输出现延迟时，该方法立刻会返回 0，导致循环结束。测试代码如下：
```
@Test
public void testWriteURL() throws Exception {
String url = "https://pan.baidu.com/static/images/16new/bg2.jpg";
long expectedSize = 77_949L;
InputStream inputStream = new URL(url).openStream();
ByteArrayOutputStream bos = new ByteArrayOutputStream();
long writeSize = org.apache.dubbo.common.utils.IOUtils.write(inputStream, bos);
assertThat(writeSize, equalTo(expectedSize)); // org.hamcrest.MatcherAssert
}
```
这个测试通常会失败，`write()` 方法在读取了部分内容后就结束循环并返回了。
正确的写法请参考 `org.apache.commons.io.IOUtils#copy(java.io.InputStream, java.io.OutputStream)` 的实现，对应的测试方法如下：
```
public static void main(String[] args) throws IOException {
String url = "https://pan.baidu.com/static/images/16new/bg2.jpg";
long expectedSize = 77_949L;
InputStream inputStream = new URL(url).openStream();
ByteArrayOutputStream bos = new ByteArrayOutputStream();
long writeSize = org.apache.commons.io.IOUtils.copy(inputStream, bos);
assertThat(writeSize, equalTo(expectedSize)); // org.hamcrest.MatcherAssert
}
```
上面这个方法就不会运行失败。
为什么这个问题一直没有暴露出来呢？因为这个方法在整个 dubbo 项目中只有一个地方用到了：`org.apache.dubbo.common.io.Bytes#unzip()`，而这个方法只用来处理内存中的字节串，而与网络无关。万一 dubbo 项目将来在某个地方用它来读取网络内容，问题就会暴露出来。
[ Full content available at: https://github.com/apache/incubator-dubbo/issues/3420 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] tswstarplanet commented on issue #3420: DO
NOT USE org.apache.dubbo.common.utils.IOUtils.write() for your own project
Posted by "tswstarplanet (GitHub)" <gi...@apache.org>.
Thanks for the issue, would you please submit a pr ?
[ Full content available at: https://github.com/apache/incubator-dubbo/issues/3420 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] kezhenxu94 commented on issue #3420: DO
NOT USE org.apache.dubbo.common.utils.IOUtils.write() for your own project
Posted by "kezhenxu94 (GitHub)" <gi...@apache.org>.
> * [x] I have searched the [issues](https://github.com/apache/incubator-dubbo/issues) of this repository and believe that this is not a duplicate.
> * [x] I have checked the [FAQ](https://github.com/apache/incubator-dubbo/blob/master/FAQ.md) of this repository and believe that this is not a duplicate.
>
> ### Environment
> * Dubbo version: 2.7.1-SNAPSHOT
>
> `org.apache.dubbo.common.utils.IOUtils` it seems that this is a class for common use but there is a pitfall in it. DO NOT USE IT IN YOUR OWN PROJECT!
>
> ### Problem Description
> There is a method called `write()` in the class：
>
> ```
> public static long write(InputStream is, OutputStream os, int bufferSize) throws IOException {
> int read;
> long total = 0;
> byte[] buff = new byte[bufferSize];
> while (is.available() > 0) { // !!!
> read = is.read(buff, 0, buff.length);
> if (read > 0) {
> os.write(buff, 0, read);
> total += read;
> }
> }
> return total;
> }
> ```
> which uses the returned value of `InputStream.available()` as the break condition of the while loop, in network environment, it returns 0 immediately in case of network latency and thus is not applicable to work as the EOF mark of the stream. Test code as follows:
>
> ```
> @Test
> public void testWriteURL() throws Exception {
> String url = "https://pan.baidu.com/static/images/16new/bg2.jpg";
> long expectedSize = 77_949L;
>
> InputStream inputStream = new URL(url).openStream();
> ByteArrayOutputStream bos = new ByteArrayOutputStream();
> long writeSize = org.apache.dubbo.common.utils.IOUtils.write(inputStream, bos);
> assertThat(writeSize, equalTo(expectedSize)); // org.hamcrest.MatcherAssert
> }
> ```
> The test will usually fail, method `write()` returns after reading part of the content and the loop breaks.
>
> for a correct implementation please refer to `org.apache.commons.io.IOUtils#copy(java.io.InputStream, java.io.OutputStream)`, and corresponding test code is:
>
> ```
> public static void main(String[] args) throws IOException {
> String url = "https://pan.baidu.com/static/images/16new/bg2.jpg";
> long expectedSize = 77_949L;
>
> InputStream inputStream = new URL(url).openStream();
> ByteArrayOutputStream bos = new ByteArrayOutputStream();
> long writeSize = org.apache.commons.io.IOUtils.copy(inputStream, bos);
> assertThat(writeSize, equalTo(expectedSize)); // org.hamcrest.MatcherAssert
> }
> ```
> the test above passes
>
> why this issue didn't expose up to now? Because it is only used once in `org.apache.dubbo.common.io.Bytes#unzip()` within the whole project, and is only used to deal with data in memory. If we use it to read data from network in the future in some places, problem occurs.
@yiding-he khanimteyaz
[ Full content available at: https://github.com/apache/incubator-dubbo/issues/3420 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] kezhenxu94 commented on issue #3420: DO
NOT USE org.apache.dubbo.common.utils.IOUtils.write() for your own project
Posted by "kezhenxu94 (GitHub)" <gi...@apache.org>.
> * [x] I have searched the [issues](https://github.com/apache/incubator-dubbo/issues) of this repository and believe that this is not a duplicate.
> * [x] I have checked the [FAQ](https://github.com/apache/incubator-dubbo/blob/master/FAQ.md) of this repository and believe that this is not a duplicate.
>
> ### Environment
> * Dubbo version: 2.7.1-SNAPSHOT
>
> `org.apache.dubbo.common.utils.IOUtils` it seems that this is a class for common use but there is a pitfall in it. DO NOT USE IT IN YOUR OWN PROJECT!
>
> ### Problem Description
> There is a method called `write()` in the class：
>
> ```
> public static long write(InputStream is, OutputStream os, int bufferSize) throws IOException {
> int read;
> long total = 0;
> byte[] buff = new byte[bufferSize];
> while (is.available() > 0) { // !!!
> read = is.read(buff, 0, buff.length);
> if (read > 0) {
> os.write(buff, 0, read);
> total += read;
> }
> }
> return total;
> }
> ```
> which uses the returned value of `InputStream.available()` as the break condition of the while loop, in network environment, it returns 0 immediately in case of network latency and thus is not applicable to work as the EOF mark of the stream. Test code as follows:
>
> ```
> @Test
> public void testWriteURL() throws Exception {
> String url = "https://pan.baidu.com/static/images/16new/bg2.jpg";
> long expectedSize = 77_949L;
>
> InputStream inputStream = new URL(url).openStream();
> ByteArrayOutputStream bos = new ByteArrayOutputStream();
> long writeSize = org.apache.dubbo.common.utils.IOUtils.write(inputStream, bos);
> assertThat(writeSize, equalTo(expectedSize)); // org.hamcrest.MatcherAssert
> }
> ```
> The test will usually fail, method `write()` returns after reading part of the content and the loop breaks.
>
> for a correct implementation please refer to `org.apache.commons.io.IOUtils#copy(java.io.InputStream, java.io.OutputStream)`, and corresponding test code is:
>
> ```
> public static void main(String[] args) throws IOException {
> String url = "https://pan.baidu.com/static/images/16new/bg2.jpg";
> long expectedSize = 77_949L;
>
> InputStream inputStream = new URL(url).openStream();
> ByteArrayOutputStream bos = new ByteArrayOutputStream();
> long writeSize = org.apache.commons.io.IOUtils.copy(inputStream, bos);
> assertThat(writeSize, equalTo(expectedSize)); // org.hamcrest.MatcherAssert
> }
> ```
> the test above passes
>
> why this issue didn't expose up to now? Because it is only used once in `org.apache.dubbo.common.io.Bytes#unzip()` within the whole project, and is only used to deal with data in memory. If we use it to read data from network in the future in some places, problem occurs.
@khanimteyaz
[ Full content available at: https://github.com/apache/incubator-dubbo/issues/3420 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3420: DO
NOT USE org.apache.dubbo.common.utils.IOUtils.write() for your own project
Posted by "khanimteyaz (GitHub)" <gi...@apache.org>.
could some one translate this for me in english. I tried but failed to it. Thanks
[ Full content available at: https://github.com/apache/incubator-dubbo/issues/3420 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] yiding-he commented on issue #3420: DO NOT
USE org.apache.dubbo.common.utils.IOUtils.write() for your own project
Posted by "yiding-he (GitHub)" <gi...@apache.org>.
@tswstarplanet OK I'll create a PR.
[ Full content available at: https://github.com/apache/incubator-dubbo/issues/3420 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org