rpc_util: added a byte slice return to recvBufferPool #6605
Conversation
rpc_util: added a byte slice return to recvBufferPool
efremovmi
changed the title
efremovmi
changed the title
|
@easwars I can't update label and milestone. Also, some tests are unstable, sometimes they work, and sometimes they don't. Should I try running tests until they stop falling? Can you help me? |
|
I've rerun the failed test |
|
@hueypark : Do you have some cycles to review this PR? |
server.go
Outdated
| @@ -1311,6 +1311,9 @@ func (s *Server) processUnaryRPC(ctx context.Context, t transport.ServerTranspor | |||
| } | |||
| return err | |||
| } | |||
|
|
|||
| defer s.opts.recvBufferPool.Put(&d) | |||
There was a problem hiding this comment.
Buffer pools shouldn't be utilized in the presence of WithStatsHandler, EnableTracing, or binary logging, and thus, they must not be returned to the pool here.
It's essential to verify whether the data is still actively utilized in the contexts mentioned above. Only consider returning it to the pool if it's no longer in use. In df function seems to be the suitable location for this implementation.
There was a problem hiding this comment.
@hueypark moved Put to a suitable location.
There was a problem hiding this comment.
Before returning d to the pool, please ensure it is not in use. Specifically, if either shs or binlogs are present, d should not be returned to the pool.
There was a problem hiding this comment.
Corrected it. See if I understood you correctly? If not, can you tell me if there is already something similar in this code base?
|
@easwars |
server.go
Outdated
| @@ -1339,6 +1339,9 @@ func (s *Server) processUnaryRPC(ctx context.Context, t transport.ServerTranspor | |||
| if trInfo != nil { | |||
| trInfo.tr.LazyLog(&payload{sent: false, msg: v}, true) | |||
| } | |||
| if len(shs) == 0 && len(binlogs) == 0 { | |||
There was a problem hiding this comment.
Should we think about adding a unit test for this conditional?
There was a problem hiding this comment.
Yes, having a unit test with a custom recv buffer pool to check that the buffer is released at the end of the RPC would be good.
There was a problem hiding this comment.
@arvindbr8 I can't figure out how to do this and how to initialize the stream for the processUnaryRPC function in order to write a unit test. Can you suggest the course of the solution?
There was a problem hiding this comment.
Maybe a more e2e style test might work better?
- Create a channel with a custom implementation of the recv buffer pool
- Perform a unary RPC
- Once the RPC is completed, ensure that the buffer is returned to your custom pool implementation
This file contains some e2e style tests which you can use as reference: https://github.com/grpc/grpc-go/blob/master/internal/idle/idle_e2e_test.go
Please let us know if you have questions. Thanks.
There was a problem hiding this comment.
@easwars Hi, there is no time to finish this task now, as there is a blockage at work and university. Can someone help with completing the task?
There was a problem hiding this comment.
@efremovmi
I've submitted a pull request and included the relevant tests. #6651
Please feel free to leverage it to finalize your pull request when you have a moment.
If you're unable to do so, just let me know, and I'll proceed with a pull request to close the issue.
There was a problem hiding this comment.
Hi! Did I understand correctly that we will make your change, and just close my branch? If so, then please take a look at my last commit and change your code. This change is important because if an error occurs during s.getCodec(stream.ContentSubtype()).Unmarshal(d, v), the buffer will not return to the pool
There was a problem hiding this comment.
Please continue on this PR.
The draft PR I created is for your reference only!
There was a problem hiding this comment.
Hi!
@hueypark @easwars @arvindbr8
Check the PR, please
|
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
efremovmi
force-pushed
the
correct_operation_of_the_pool
branch
from
cf5359c to
cb430be
Compare
|
@easwars That's it, did I do it right? |
|
@efremovmi -- did you mean to close this PR? or close and reopen to force the pre-merge checkers to re-run? FYI: easwars is out on parental leave. But we will have others on the team to pick this review up. |
|
@arvindbr8 as far as I understood, my corrections were made to the experimental package and in fact this branch is no longer needed (I sent the master to myself and the request pulll closed itself) |
|
Although I may have messed up something...Probably I need to make that change again? |
Would you like us to re-open the issue in that case? @efremovmi |
|
@arvindbr8 Is there an opportunity to continue in this PR? so that the history is stored completely (this is my first PR in open source, I do not know what to do :)) |
|
@efremovmi - mmm, that last force-push seems to have messed up the commit history on your branch. Right now seems like you lost all of the commits made to do you happen to have a backup of the branch which you could use to restore to the previous snapshot? Feels like you might need some dark git magic to bring your branch back from the grave. 😢 |
|
@efremovmi If you don't have time to do this now, I'll take over. What do you think? |
|
@hueypark yes, sorry, it's hard to combine work and study. I would be glad if you finished this project |
|
@efremovmi -- I would suggest that if you have the time to run |
…removmi/grpc-go into correct_operation_of_the_pool
…removmi/grpc-go into correct_operation_of_the_pool
|
@arvindbr8 done |
|
Nice! Thanks. Assigning this PR to @hueypark |
|
I've created a new PR to cover this issue! Let's continue the discussion from there. #6766 |
|
Closing this in favor of #6766 |
This PR corrects the operation of the buffer pool for unary methods.
This type of fix adds a buffer back to the pool when used. See more details #6578
RELEASE NOTES: none