You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Stephan Erb <se...@apache.org> on 2016/02/14 21:35:15 UTC
Review Request 43567: Always close Deflater/Inflater streams
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43567/
-----------------------------------------------------------
Review request for Aurora, John Sirois and Zameer Manji.
Repository: aurora
Description
-------
Closing Deflater/Inflater streams explicitly frees their native memory instantly. Otherwise, the memory will only be freed once the object finalizer runs.
I got the idea from this article http://www.evanjones.ca/java-native-leak-bug.html. Unfortunately, we cannot use the Java close-with-resource feature, as `TTransport` in our current Thrift version does not implement `Closable`.
Diffs
-----
src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java 6b65c0f6cb60b98ff352c4c5d5fed38d52b4b062
Diff: https://reviews.apache.org/r/43567/diff/
Testing
-------
./gradlew -Pq build
Thanks,
Stephan Erb
Re: Review Request 43567: Always close Deflater/Inflater streams
Posted by Stephan Erb <se...@apache.org>.
> On Feb. 14, 2016, 9:37 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java, line 164
> > <https://reviews.apache.org/r/43567/diff/1/?file=1241353#file1241353line164>
> >
> > Note: this `close` cannot be moved to `finally` and cannot be replaced by a `flush`. It is mandatory at this position.
>
> Bill Farner wrote:
> I presume this is because closing has a flush effect that renders `outBytes` complete? A one-line comment to that effect would go a long way.
>
> However, can you additionally do the `close()` in a finally without causing harm? (and omit from the `catch` branch)
Good call. Done.
- Stephan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43567/#review119187
-----------------------------------------------------------
On Feb. 14, 2016, 10:46 p.m., Stephan Erb wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43567/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2016, 10:46 p.m.)
>
>
> Review request for Aurora, John Sirois and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Closing Deflater/Inflater streams explicitly frees their native memory instantly. Otherwise, the memory will only be freed once the object finalizer runs.
>
> I got the idea from this article http://www.evanjones.ca/java-native-leak-bug.html. Unfortunately, we cannot use the Java close-with-resource feature, as `TTransport` in our current Thrift version does not implement `Closable`.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java 6b65c0f6cb60b98ff352c4c5d5fed38d52b4b062
>
> Diff: https://reviews.apache.org/r/43567/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Stephan Erb
>
>
Re: Review Request 43567: Always close Deflater/Inflater streams
Posted by Bill Farner <wf...@apache.org>.
> On Feb. 14, 2016, 12:37 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java, line 164
> > <https://reviews.apache.org/r/43567/diff/1/?file=1241353#file1241353line164>
> >
> > Note: this `close` cannot be moved to `finally` and cannot be replaced by a `flush`. It is mandatory at this position.
I presume this is because closing has a flush effect that renders `outBytes` complete? A one-line comment to that effect would go a long way.
However, can you additionally do the `close()` in a finally without causing harm? (and omit from the `catch` branch)
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43567/#review119187
-----------------------------------------------------------
On Feb. 14, 2016, 12:35 p.m., Stephan Erb wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43567/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2016, 12:35 p.m.)
>
>
> Review request for Aurora, John Sirois and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Closing Deflater/Inflater streams explicitly frees their native memory instantly. Otherwise, the memory will only be freed once the object finalizer runs.
>
> I got the idea from this article http://www.evanjones.ca/java-native-leak-bug.html. Unfortunately, we cannot use the Java close-with-resource feature, as `TTransport` in our current Thrift version does not implement `Closable`.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java 6b65c0f6cb60b98ff352c4c5d5fed38d52b4b062
>
> Diff: https://reviews.apache.org/r/43567/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Stephan Erb
>
>
Re: Review Request 43567: Always close Deflater/Inflater streams
Posted by Stephan Erb <se...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43567/#review119187
-----------------------------------------------------------
src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java (line 162)
<https://reviews.apache.org/r/43567/#comment180468>
Note: this `close` cannot be moved to `finally` and cannot be replaced by a `flush`. It is mandatory at this position.
- Stephan Erb
On Feb. 14, 2016, 9:35 p.m., Stephan Erb wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43567/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2016, 9:35 p.m.)
>
>
> Review request for Aurora, John Sirois and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Closing Deflater/Inflater streams explicitly frees their native memory instantly. Otherwise, the memory will only be freed once the object finalizer runs.
>
> I got the idea from this article http://www.evanjones.ca/java-native-leak-bug.html. Unfortunately, we cannot use the Java close-with-resource feature, as `TTransport` in our current Thrift version does not implement `Closable`.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java 6b65c0f6cb60b98ff352c4c5d5fed38d52b4b062
>
> Diff: https://reviews.apache.org/r/43567/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Stephan Erb
>
>
Re: Review Request 43567: Always close Deflater/Inflater streams
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43567/#review119193
-----------------------------------------------------------
Ship it!
Ship It!
- Bill Farner
On Feb. 14, 2016, 1:46 p.m., Stephan Erb wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43567/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2016, 1:46 p.m.)
>
>
> Review request for Aurora, John Sirois and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Closing Deflater/Inflater streams explicitly frees their native memory instantly. Otherwise, the memory will only be freed once the object finalizer runs.
>
> I got the idea from this article http://www.evanjones.ca/java-native-leak-bug.html. Unfortunately, we cannot use the Java close-with-resource feature, as `TTransport` in our current Thrift version does not implement `Closable`.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java 6b65c0f6cb60b98ff352c4c5d5fed38d52b4b062
>
> Diff: https://reviews.apache.org/r/43567/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Stephan Erb
>
>
Re: Review Request 43567: Always close Deflater/Inflater streams
Posted by Stephan Erb <se...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43567/
-----------------------------------------------------------
(Updated Feb. 14, 2016, 10:46 p.m.)
Review request for Aurora, John Sirois and Zameer Manji.
Changes
-------
Clarifications proposed by Bill.
Repository: aurora
Description
-------
Closing Deflater/Inflater streams explicitly frees their native memory instantly. Otherwise, the memory will only be freed once the object finalizer runs.
I got the idea from this article http://www.evanjones.ca/java-native-leak-bug.html. Unfortunately, we cannot use the Java close-with-resource feature, as `TTransport` in our current Thrift version does not implement `Closable`.
Diffs (updated)
-----
src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java 6b65c0f6cb60b98ff352c4c5d5fed38d52b4b062
Diff: https://reviews.apache.org/r/43567/diff/
Testing
-------
./gradlew -Pq build
Thanks,
Stephan Erb
Re: Review Request 43567: Always close Deflater/Inflater streams
Posted by Stephan Erb <se...@apache.org>.
> On Feb. 14, 2016, 9:59 p.m., Aurora ReviewBot wrote:
> > Master (2b48f22) is red with this patch.
> > ./build-support/jenkins/build.sh
> >
> > status = self.run(options, args)
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/commands/install.py", line 299, in run
> > requirement_set.prepare_files(finder)
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/req/req_set.py", line 359, in prepare_files
> > ignore_dependencies=self.ignore_dependencies))
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/req/req_set.py", line 576, in _prepare_file
> > session=self.session, hashes=hashes)
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/download.py", line 809, in unpack_url
> > hashes=hashes
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/download.py", line 648, in unpack_http_url
> > hashes)
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/download.py", line 841, in _download_http_url
> > stream=True,
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/_vendor/requests/sessions.py", line 480, in get
> > return self.request('GET', url, **kwargs)
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/download.py", line 377, in request
> > return super(PipSession, self).request(method, url, *args, **kwargs)
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/_vendor/requests/sessions.py", line 468, in request
> > resp = self.send(prep, **send_kwargs)
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/_vendor/requests/sessions.py", line 576, in send
> > r = adapter.send(request, **kwargs)
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/_vendor/cachecontrol/adapter.py", line 46, in send
> > resp = super(CacheControlAdapter, self).send(request, **kw)
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/_vendor/requests/adapters.py", line 447, in send
> > raise SSLError(e, request=request)
> > SSLError: [Errno 185090050] _ssl.c:344: error:0B084002:x509 certificate routines:X509_load_cert_crl_file:system lib
> > ----------------------------------------
> > ...Installing setuptools, pip, wheel...done.
> > Traceback (most recent call last):
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv.py", line 2284, in <module>
> > main()
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv.py", line 703, in main
> > symlink=options.symlink)
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv.py", line 904, in create_environment
> > download=download,
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv.py", line 861, in install_wheel
> > call_subprocess(cmd, show_stdout=False, extra_env=env)
> > File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv.py", line 781, in call_subprocess
> > % (cmd_desc, proc.returncode))
> > OSError: Command /home/jenkins/jenkin...s.venv/bin/python2.7 -c "import sys, pip; sys...d\"] + sys.argv[1:]))" setuptools pip wheel failed with error code 2
> >
> >
> > I will refresh this build result if you post a review containing "@ReviewBot retry"
This error is due to a buggy virtualenv version. We should update to 14.0.6 https://virtualenv.pypa.io/en/latest/changes.html#id1
- Stephan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43567/#review119188
-----------------------------------------------------------
On Feb. 14, 2016, 9:35 p.m., Stephan Erb wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43567/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2016, 9:35 p.m.)
>
>
> Review request for Aurora, John Sirois and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Closing Deflater/Inflater streams explicitly frees their native memory instantly. Otherwise, the memory will only be freed once the object finalizer runs.
>
> I got the idea from this article http://www.evanjones.ca/java-native-leak-bug.html. Unfortunately, we cannot use the Java close-with-resource feature, as `TTransport` in our current Thrift version does not implement `Closable`.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java 6b65c0f6cb60b98ff352c4c5d5fed38d52b4b062
>
> Diff: https://reviews.apache.org/r/43567/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Stephan Erb
>
>
Re: Review Request 43567: Always close Deflater/Inflater streams
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43567/#review119188
-----------------------------------------------------------
Master (2b48f22) is red with this patch.
./build-support/jenkins/build.sh
status = self.run(options, args)
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/commands/install.py", line 299, in run
requirement_set.prepare_files(finder)
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/req/req_set.py", line 359, in prepare_files
ignore_dependencies=self.ignore_dependencies))
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/req/req_set.py", line 576, in _prepare_file
session=self.session, hashes=hashes)
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/download.py", line 809, in unpack_url
hashes=hashes
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/download.py", line 648, in unpack_http_url
hashes)
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/download.py", line 841, in _download_http_url
stream=True,
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/_vendor/requests/sessions.py", line 480, in get
return self.request('GET', url, **kwargs)
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/download.py", line 377, in request
return super(PipSession, self).request(method, url, *args, **kwargs)
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/_vendor/requests/sessions.py", line 468, in request
resp = self.send(prep, **send_kwargs)
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/_vendor/requests/sessions.py", line 576, in send
r = adapter.send(request, **kwargs)
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/_vendor/cachecontrol/adapter.py", line 46, in send
resp = super(CacheControlAdapter, self).send(request, **kw)
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl/pip/_vendor/requests/adapters.py", line 447, in send
raise SSLError(e, request=request)
SSLError: [Errno 185090050] _ssl.c:344: error:0B084002:x509 certificate routines:X509_load_cert_crl_file:system lib
----------------------------------------
...Installing setuptools, pip, wheel...done.
Traceback (most recent call last):
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv.py", line 2284, in <module>
main()
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv.py", line 703, in main
symlink=options.symlink)
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv.py", line 904, in create_environment
download=download,
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv.py", line 861, in install_wheel
call_subprocess(cmd, show_stdout=False, extra_env=env)
File "/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.5/virtualenv.py", line 781, in call_subprocess
% (cmd_desc, proc.returncode))
OSError: Command /home/jenkins/jenkin...s.venv/bin/python2.7 -c "import sys, pip; sys...d\"] + sys.argv[1:]))" setuptools pip wheel failed with error code 2
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Feb. 14, 2016, 8:35 p.m., Stephan Erb wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43567/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2016, 8:35 p.m.)
>
>
> Review request for Aurora, John Sirois and Zameer Manji.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Closing Deflater/Inflater streams explicitly frees their native memory instantly. Otherwise, the memory will only be freed once the object finalizer runs.
>
> I got the idea from this article http://www.evanjones.ca/java-native-leak-bug.html. Unfortunately, we cannot use the Java close-with-resource feature, as `TTransport` in our current Thrift version does not implement `Closable`.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java 6b65c0f6cb60b98ff352c4c5d5fed38d52b4b062
>
> Diff: https://reviews.apache.org/r/43567/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Stephan Erb
>
>