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

Fix memory leak #1197

Merged
merged 2 commits into from Jun 3, 2020
Merged

Fix memory leak #1197

merged 2 commits into from Jun 3, 2020

Conversation

ppressives
Copy link
Contributor

@ppressives ppressives commented Jun 2, 2020

After upgrading Monix to the latest version I found a memory leak related to the new bracket implementation.

Git bisect shows the culprit commit:
#1120

Below is a simple piece of code that reproduces the issue:
F.guarantee(F.unit)(F.unit).foreverM - it fails with OutOfMemoryError on monix.eval.Task, but works fine with cats.effect.IO

I'm not sure if this is a correct fix of the issue and I need help to write a test on this case

@@ -190,7 +190,7 @@ private[monix] object TaskBracket {
val fb =
try use(value)
catch { case NonFatal(e) => Task.raiseError(e) }
fb.flatMap(releaseFrame)
fb.flatMap(releaseFrame).flatMap(r => Task { conn.pop(); r })
Copy link
Collaborator

@Avasil Avasil Jun 2, 2020

Choose a reason for hiding this comment

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

I feel like we could call ctx.connection.pop() in unsafeRecover and unsafeApply which I removed by mistake in the PR you mentioned

Copy link
Contributor Author

@ppressives ppressives Jun 2, 2020

Choose a reason for hiding this comment

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

If I put this call there, tests fail. I think it breaks the feature you did in that PR.

Copy link
Collaborator

@Avasil Avasil Jun 2, 2020

Choose a reason for hiding this comment

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

Ah, I see.

I think we could do it in a similar way to Cats IO: https://github.com/typelevel/cats-effect/blob/master/core/shared/src/main/scala/cats/effect/internals/IOBracket.scala#L189

so

  private[this] val disableUncancelableAndPop: (Any, Throwable, Context, Context) => Context =
    (_, _, old, _) => {
      old.connection.pop()
      old
    }

instead of a null in ContextSwitch.

I'm not able to check right now but the testa should pass since they are passing for Cats IO and hopefully the memory leak will go away :)

Copy link
Contributor Author

@ppressives ppressives Jun 3, 2020

Choose a reason for hiding this comment

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

Sure, it works fine. Thank you!

@Avasil Avasil added this to the 3.2.2 milestone Jun 2, 2020
@Avasil
Copy link
Collaborator

Avasil commented Jun 2, 2020

Big thank you!

I'm not sure if this is a correct fix of the issue and I need help to write a test on this case

I think we're fine skipping the test if it takes a long time to go out of memory

@ppressives ppressives changed the title WIP: Fix memory leak Fix memory leak Jun 2, 2020
Avasil
Avasil approved these changes Jun 3, 2020
Copy link
Collaborator

@Avasil Avasil left a comment

Thank you!

Do you need a snapshot version release / official release right now?
I'm hoping to finish everything for 3.2.2 release within 1-2 weeks but I am considering to pull the trigger earlier considering the severity of the issue

@ppressives
Copy link
Contributor Author

ppressives commented Jun 3, 2020

I rolled back Monix to 3.1.0 in my project, so it can wait for some time.

@Avasil Avasil merged commit 83745ca into monix:master Jun 3, 2020
1 check passed
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.

None yet

2 participants