uber / tchannel-go

Go implementation of a multiplexing and framing protocol for RPC calls
http://uber.github.io/tchannel/
MIT License
490 stars 84 forks source link

Release unsent frames when flushing fragments #887

Closed cinchurge closed 2 years ago

cinchurge commented 2 years ago

Currently, when sending mutated frames via Relayer.fragmentingSend() frames that aren't successfully sent (e.g. due to send buffer being full during fragment flush) aren't returned to the frame pool and end up piling up until either OOM or the connection gets closed.

This change adds a release call after marking the relay item as failed to ensure that frames are properly returned to the pool.

codecov[bot] commented 2 years ago

Codecov Report

Merging #887 (24f5ea7) into dev (9c59f88) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##              dev     #887   +/-   ##
=======================================
  Coverage   89.25%   89.25%           
=======================================
  Files          43       43           
  Lines        4439     4440    +1     
=======================================
+ Hits         3962     3963    +1     
  Misses        361      361           
  Partials      116      116           
Impacted Files Coverage Δ
relay.go 86.14% <100.00%> (+0.45%) :arrow_up:
root_peer_list.go 92.00% <0.00%> (-4.00%) :arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

DheerendraRathor commented 2 years ago

@lambdai

All other flushFragment() calls in fragmenting_writer.go WERE always followed by a newFragment Both occurrences seems to put the curFragment in a to-be-released state.

From what I understand, these calls were followed by newFragment where flushFragment is successful i.e. connection will release them once they are written to network. https://github.com/uber/tchannel-go/blob/dev/connection.go#L687

In case there are errors in flushFragment, Eric's change will take care of releasing them which were earlier contributing to leaks.