You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by Ben Craig <be...@apache.org> on 2013/09/13 20:57:33 UTC
Review Request 14133: THRIFT-1201: getaddrinfo resource leak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14133/
-----------------------------------------------------------
Review request for Thrift.
Bugs: THRIFT-1201
https://issues.apache.org/jira/browse/THRIFT-1201
Repository: Thrift
Description
-------
THRIFT-1201: getaddrinfo resource leak
Client: cpp
Patch: Frank Meerkoetter
Diffs
-----
lib/cpp/Makefile.am 7b879f3
lib/cpp/src/thrift/server/TNonblockingServer.cpp b9553c4
lib/cpp/src/thrift/transport/TServerSocket.cpp 59a0885
lib/cpp/src/thrift/util/TResourceHolder.h PRE-CREATION
lib/cpp/src/thrift/util/TSocketHelper.h PRE-CREATION
lib/cpp/src/thrift/util/TSocketHelper.cpp PRE-CREATION
lib/cpp/test/Makefile.am d04cfb5
lib/cpp/test/TServerSocketTest.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/14133/diff/
Testing
-------
Windows testing in progress
Thanks,
Ben Craig
Re: Review Request 14133: THRIFT-1201: getaddrinfo resource leak
Posted by Ben Craig <be...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14133/#review26090
-----------------------------------------------------------
lib/cpp/src/thrift/transport/TServerSocket.cpp
<https://reviews.apache.org/r/14133/#comment50917>
Some funky white space here. I think there's a tab mixed in with the spaces.
lib/cpp/src/thrift/util/TResourceHolder.h
<https://reviews.apache.org/r/14133/#comment50918>
This gets the job done, but I think it is overgeneralizing things, and the generalization makes it more painful to use. I think it would be reasonable to tie this class specifically to addrinfo. Then you don't need to template anything, clients don't need to pass in a FREE_T, and your class won't need a resourceIsValid_ bool, it can just check for NULL.
lib/cpp/src/thrift/util/TSocketHelper.h
<https://reviews.apache.org/r/14133/#comment50919>
The thrift codebase recently switched to using < > instead of " " for pulling in thrift headers, because config.h was causing such pain. This should be < >.
lib/cpp/src/thrift/util/TSocketHelper.cpp
<https://reviews.apache.org/r/14133/#comment50920>
< >
lib/cpp/src/thrift/util/TSocketHelper.cpp
<https://reviews.apache.org/r/14133/#comment50921>
spaces here instead of tabs
lib/cpp/test/TServerSocketTest.cpp
<https://reviews.apache.org/r/14133/#comment50923>
Thank you for considering testing for your change. But...
I don't think this exercises your code changes. I don't think it adds much coverage over the existing transport tests. I'm fine with reverting this file and the associated test/Makefile.am.
lib/cpp/test/TServerSocketTest.cpp
<https://reviews.apache.org/r/14133/#comment50922>
Most thrift tests use port 9090. Not a showstopper though.
- Ben Craig
On Sept. 13, 2013, 6:57 p.m., Ben Craig wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14133/
> -----------------------------------------------------------
>
> (Updated Sept. 13, 2013, 6:57 p.m.)
>
>
> Review request for Thrift.
>
>
> Bugs: THRIFT-1201
> https://issues.apache.org/jira/browse/THRIFT-1201
>
>
> Repository: Thrift
>
>
> Description
> -------
>
> THRIFT-1201: getaddrinfo resource leak
> Client: cpp
> Patch: Frank Meerkoetter
>
>
> Diffs
> -----
>
> lib/cpp/Makefile.am 7b879f3
> lib/cpp/src/thrift/server/TNonblockingServer.cpp b9553c4
> lib/cpp/src/thrift/transport/TServerSocket.cpp 59a0885
> lib/cpp/src/thrift/util/TResourceHolder.h PRE-CREATION
> lib/cpp/src/thrift/util/TSocketHelper.h PRE-CREATION
> lib/cpp/src/thrift/util/TSocketHelper.cpp PRE-CREATION
> lib/cpp/test/Makefile.am d04cfb5
> lib/cpp/test/TServerSocketTest.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/14133/diff/
>
>
> Testing
> -------
>
> Windows testing in progress
>
>
> Thanks,
>
> Ben Craig
>
>