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.

Sign up for GitHub

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

Jump to bottom

erts: Fix fragmented send to finish before exiting #5892

Merged
merged 8 commits into from
May 2, 2022
Merged

erts: Fix fragmented send to finish before exiting #5892

merged 8 commits into from
May 2, 2022

Conversation

garazdawi
Copy link
Contributor

If a process is suspended while doing a fragmented send and then receives an exit signal it was terminated before it could
finish sending the message leading to a memory leak on the receiving side.

This change fixes so that the message is allowed to finish being sent before the process exits.

Closes #5876

@garazdawi garazdawi added team:VM Assigned to OTP team VM fix labels Apr 14, 2022
@garazdawi garazdawi added this to the OTP-25.0 milestone Apr 14, 2022
@garazdawi garazdawi self-assigned this Apr 14, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 14, 2022 •
edited
Loading

CT Test Results

       3 files     125 suites   38m 20s ⏱️
1 444 tests 1 402 ✔️ 42 💤 0 ❌
1 743 runs  1 684 ✔️ 59 💤 0 ❌

Results for commit 6f974a8.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@garazdawi garazdawi mentioned this pull request Apr 14, 2022
Closed
@nickva
Copy link
Contributor

nickva commented Apr 15, 2022

Confirming that this PR fixed the binary leak we had in production: #5876 (comment)

garazdawi
garazdawi commented Apr 19, 2022
View reviewed changes
erts/emulator/beam/erl_process.c Outdated
@@ -13897,6 +13900,9 @@ erts_continue_exit_process(Process *p)
/* notify free */
erts_atomic32_read_bor_relb(&p->state, ERTS_PSFLG_FREE);

/* clear suspend count */
p->rcount = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out that doing this is not always safe, so need to implement another way to deal with the suspend count.

@garazdawi garazdawi requested a review from rickard-green April 20, 2022 20:19
@garazdawi garazdawi marked this pull request as ready for review April 20, 2022 20:19
@garazdawi
Copy link
Contributor Author

The test case needs more work, but I think I'm happy with this solution.

@garazdawi garazdawi requested a review from sverker April 20, 2022 20:20
@garazdawi garazdawi force-pushed the lukas/erts/fix-dist-fragment-exit-leak branch from 9e0873b to afcbdf7 Compare April 21, 2022 06:59
@garazdawi garazdawi added the testing currently being tested, tag is used by OTP internal CI label Apr 21, 2022
@garazdawi
Copy link
Contributor Author

Fixed the test case to test what it should test.

@garazdawi garazdawi removed the testing currently being tested, tag is used by OTP internal CI label Apr 21, 2022
sverker
sverker reviewed Apr 25, 2022
View reviewed changes
erts/emulator/beam/erl_process.c Outdated Show resolved Hide resolved
erts/emulator/beam/erl_process.c Show resolved Hide resolved
@garazdawi
erts: Fix fragmented send to finish before exiting
c78a8e9 
If a process is suspended doing a fragmented send and then
receives an exit signal it was terminated before it could
finish sending the message leading to a memory leak on the
receiving side.

This change fixes that so that the message is allowed to
finish being sent before the process exits.

Closes erlang#5876
@garazdawi
erts: Add etp-links for inspecting process links
6b12452
@garazdawi
erts: Fix size calculation of suspend monitor
c67375e
@garazdawi garazdawi force-pushed the lukas/erts/fix-dist-fragment-exit-leak branch 2 times, most recently from 97a9c2f to dc7cf3b Compare April 27, 2022 12:38
@garazdawi
erts: Add testcase for exit distr fragments
2cc59af
@garazdawi
erts: Fix faulty alloc assert on 32-bit
864cfc5 
The total block size can be > 2 GB, so the ASSERT would trigger
on working code. So we only check the blocks size on 64-bit
systems.
@garazdawi
Merge branch 'lukas/22/erts/fix-dist-fragment-exit-leak/OTP-18077' in…
9b349a5 
…to lukas/23/erts/fix-dist-fragment-exit-leak/OTP-18077
@garazdawi
Merge branch 'lukas/23/erts/fix-dist-fragment-exit-leak/OTP-18077' in…
94cff41 
…to lukas/24/erts/fix-dist-fragment-exit-leak/OTP-18077
@garazdawi
Merge branch 'lukas/24/erts/fix-dist-fragment-exit-leak/OTP-18077' in…
6f974a8 
…to lukas/25/erts/fix-dist-fragment-exit-leak/OTP-18077
@garazdawi garazdawi force-pushed the lukas/erts/fix-dist-fragment-exit-leak branch from c4525a9 to 6f974a8 Compare May 1, 2022 15:46
@garazdawi garazdawi merged commit 78b6a02 into erlang:master May 2, 2022
@garazdawi
Copy link
Contributor Author

Fix merged to master. It will also be part of the next patches to all currently supported releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sverker sverker sverker left review comments

@nickva nickva nickva left review comments

@rickard-green rickard-green Awaiting requested review from rickard-green

Assignees

@garazdawi garazdawi

Labels
fix team:VM Assigned to OTP team VM
Projects
None yet
Milestone
OTP-25.0
Development

Successfully merging this pull request may close these issues.

Binary memory leak in drv_binary associated with the dist port
3 participants
@garazdawi @nickva @sverker