thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (THRIFT-4624) Refc binary leak in binary and compact protocols
Date Sun, 02 Sep 2018 11:33:00 GMT

    [ https://issues.apache.org/jira/browse/THRIFT-4624?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16601182#comment-16601182
] 

ASF GitHub Bot commented on THRIFT-4624:
----------------------------------------

k32 commented on a change in pull request #1585: THRIFT-4624: Fix refc binary leak
URL: https://github.com/apache/thrift/pull/1585#discussion_r214537486
 
 

 ##########
 File path: lib/erl/src/thrift_compact_protocol.erl
 ##########
 @@ -363,26 +365,32 @@ read(This0, double) ->
 % returns a binary directly, call binary_to_list if necessary
 read(This0, string) ->
   {This1, {ok, Sz}}  = read(This0, ui32),
-  read_data(This1, Sz).
+  read_data(This1, Sz, This0#t_compact.clone_binaries).
 
--spec read_data(#t_compact{}, non_neg_integer()) ->
+-spec read_data(#t_compact{}, non_neg_integer(), boolean()) ->
     {#t_compact{}, {ok, binary()} | {error, _Reason}}.
-read_data(This, 0) -> {This, {ok, <<>>}};
-read_data(This = #t_compact{transport = Trans}, Len) when is_integer(Len) andalso Len >
0 ->
-    {NewTransport, Result} = thrift_transport:read(Trans, Len),
+read_data(This, 0, _) -> {This, {ok, <<>>}};
+read_data(This, Len, Clone) when is_integer(Len) andalso Len > 0 ->
+    #t_compact{transport = Trans} = This,
+    {NewTransport, Result0} = thrift_transport:read(Trans, Len),
+    Result = case Result0 of
+                 {ok, Bin} when is_binary(Bin), Clone, Len > 64 ->
 
 Review comment:
   I'm not sure how one can reliably test this, as refc/heap binary dichotomy is hidden in
the Erlang VM internals and there is no API to check which is which. The only (known to me)
way to detect refc memory leak is through memory region monitoring, and it's beyond scope
of unit/component tests. So regression testing is probably the best we can do.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


> Refc binary leak in binary and compact protocols
> ------------------------------------------------
>
>                 Key: THRIFT-4624
>                 URL: https://issues.apache.org/jira/browse/THRIFT-4624
>             Project: Thrift
>          Issue Type: Improvement
>          Components: Erlang - Library
>            Reporter: something
>            Priority: Major
>
> Pattern-matching on large (>64B) Erlang binaries merely produces slices of objects
on the Refc heap. Therefore Thrift binary and compact protocols should clone all binaries
they send to upper levels, otherwise there's a chance that transport-level messages will be
never freed.
> Patch is underway.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message