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

Bail out of the loop when the stub Input stream is done #231

Merged

Conversation

chaosbox
Copy link
Contributor

This PR fixes

  • the OOM caused by unnecessary creation of timer objects even after the request/response is completed.
    Find pprof attached that shows before and after the fix.

Before:
image

After:
image

@xthexder
Copy link
Contributor

Thanks for the fix! < 2MB is definitely a much more reasonable amount of memory usage.

I'll take a closer look later tonight and get this merged.

@chaosbox chaosbox mentioned this pull request Oct 18, 2018
@fantayeneh
Copy link

Ola @xthexder, can we merge this?

@chaosbox
Copy link
Contributor Author

chaosbox commented Nov 9, 2018

@jpittis @xthexder
hi guys,
any feedback on this PR?
thanks

Copy link
Contributor

@jpittis jpittis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM.

@pgeorgiev333
Copy link

Can you guys merge this and create a new release? This is pretty important as we cannot simulate network failures for longer periods of time.
Thanks in advance

case <-stub.Input:
case c := <-stub.Input:
if c == nil {
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure this needs a stub.Close().

@ghost ghost added the cla-needed label Dec 17, 2018
@jpittis
Copy link
Contributor

jpittis commented Dec 17, 2018

So I'm pretty sure this is good to merge now.

Any chance you can sanity check @xthexder?

Unfortunately, our company recently rolled out this contributor license agreement stuff so I can't merge this until the original contributor has clicked through the agreement. @chaosbox

I'm so sorry this has taken so long to ship, I'll release this as soon as we merge.

@chaosbox
Copy link
Contributor Author

@jpittis I have accepted the CLA for the emailID tied to my public github account.

@jpittis
Copy link
Contributor

jpittis commented Dec 17, 2018

I think you're going to have to change your commit to be authored by the same public email.

@xthexder
Copy link
Contributor

This looks good to me.
I haven't really had much time to look over Toxiproxy recently, I may try to find some time over the holidays to do a pass and fix any outstanding issues.

@xthexder xthexder self-requested a review December 17, 2018 21:21
@chaosbox chaosbox force-pushed the timeout_toxic_creates_unnecessary_timers branch from 11d4968 to 1390d99 Compare December 18, 2018 01:33
@ghost ghost removed the cla-needed label Dec 18, 2018
@jpittis jpittis merged commit 7eb6f30 into Shopify:master Dec 18, 2018
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.

5 participants