Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed a memory leak in the Retry functionality for EVM clients #909

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

rokn
Copy link
Contributor

@rokn rokn commented Aug 15, 2023

Detailed description:
The retry loop was leaking go routine from the flow which was not finished(either the timeout or the executionFunction because when one finished the other couldn't send a message on it's channel.

The solution is to just use a context with a timeout and leave the handling to the user which works perfectly in our case as everywhere we are using this we are already using a context.

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Merging #909 (9af038f) into main (08b8458) will increase coverage by 0.14%.
The diff coverage is 0.00%.

❗ Current head 9af038f differs from pull request most recent head 0d0c186. Consider uploading reports for the commit 0d0c186 to get more accurate results

@@            Coverage Diff             @@
##             main     #909      +/-   ##
==========================================
+ Coverage   64.15%   64.29%   +0.14%     
==========================================
  Files          86       86              
  Lines        6731     6716      -15     
==========================================
  Hits         4318     4318              
+ Misses       2199     2184      -15     
  Partials      214      214              
Flag Coverage Δ
unittests 64.29% <0.00%> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
app/clients/evm/client.go 56.36% <0.00%> (+4.69%) ⬆️

app/domain/service/retry.go Outdated Show resolved Hide resolved
app/domain/service/retry.go Show resolved Hide resolved
asenslime
asenslime previously approved these changes Aug 15, 2023
Coiling-Dragon
Coiling-Dragon previously approved these changes Aug 18, 2023
@rokn rokn merged commit 9eb5524 into main Aug 18, 2023
4 of 5 checks passed
@rokn rokn deleted the fix/retry-memory-leak branch August 18, 2023 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants