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 #2762 - eval after zip may cause interruption #2763

Merged
merged 1 commit into from
Dec 23, 2021

Conversation

mpilquist
Copy link
Member

@mpilquist mpilquist commented Dec 22, 2021

In the fix for #2717, we discovered that sometimes after zipping, a closed scope is used for evaluation. We fixed #2717 by ensuring that manual interruption checks occur on the nearest open scope in the case the current scope has been closed already. We needed a similar check for effect evaluation, which checks interruption inside interruptibleEval.

@mpilquist mpilquist merged commit ebb169f into typelevel:main Dec 23, 2021
@mpilquist mpilquist deleted the bugfix/2762 branch December 23, 2021 13:44
@diesalbla
Copy link
Contributor

diesalbla commented Dec 24, 2021

@mpilquist So, I have noticed that this Pull Request has broken the memory leak tests. In particular, the test broadcastThrough identity and observe + zip seems to have broken after the change.

Edit those memory leak tests were being run with the Graal VM, OpenJDK Runtime Environment GraalVM CE 21.3.0 (build 17.0.1+12-jvmci-21.3-b05).

@mpilquist
Copy link
Member Author

mpilquist commented Dec 24, 2021

I see these failures on HEAD - 2173855:

==> X fs2.MemoryLeakSpec.parJoin
==> X fs2.MemoryLeakSpec.broadcastThrough identity

Moving back a few commits to 61b593c:

==> X fs2.MemoryLeakSpec.parJoin
==> X fs2.MemoryLeakSpec.hung merge
==> X fs2.MemoryLeakSpec.broadcastThrough identity

v3.2.0 is good

b811c67 is bad:

==> X fs2.MemoryLeakSpec.parJoin
==> X fs2.MemoryLeakSpec.broadcastThrough identity
==> X fs2.MemoryLeakSpec.observe + zip

0d3ef3b is good

2861a4e is bad:

==> X fs2.MemoryLeakSpec.drain onComplete
==> X fs2.MemoryLeakSpec.hung merge
==> X fs2.MemoryLeakSpec.attempts + pull

d508c4c is good

0d6f6cb is good

7268392 is bad:

==> X fs2.MemoryLeakSpec.hung merge
==> X fs2.MemoryLeakSpec.retry

df62117 is good

3b5edc5 is good

Offending commit:

72683927716d3eb666fd507dd4ff717c9f233349 is the first bad commit
commit 72683927716d3eb666fd507dd4ff717c9f233349
Author: danicheg <e.danicheg@yandex.ru>
Date:   Sun Nov 28 14:55:09 2021 +0300

    Bump CE version to 3.3.0

 build.sbt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

@armanbilge
Copy link
Member

armanbilge commented Dec 24, 2021

Hmm, seems plausible to me that these tests may need to be adjusted, due to the increased memory pressure caused by fiber dumps. Run with tracing disabled?

@mpilquist
Copy link
Member Author

Yeah, I'm trying to disable tracing on the integration tests now

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.

Unexpected behaviors around the use of Pull
3 participants