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

os_mon: Close cpu_sup and memsup port on terminate #6689

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

jdamanalo
Copy link
Contributor

Closes #4221

There seems to be a similar issue with memsup. Should I include it here?

@jdamanalo jdamanalo changed the title Close cpu_sup socket on terminate Close cpu_sup port on terminate Jan 16, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 16, 2023

CT Test Results

    2 files    15 suites   3m 30s ⏱️
111 tests 108 ✔️ 3 💤 0
130 runs  127 ✔️ 3 💤 0

Results for commit 00cfb44.

♻️ 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

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Jan 23, 2023
@rickard-green rickard-green self-assigned this Jan 23, 2023
@KennethL KennethL added this to the OTP-26.0-rc1 milestone Feb 2, 2023
@jdamanalo jdamanalo force-pushed the os_mon/cpu_sup-close branch 2 times, most recently from 5abc3b8 to 22ae251 Compare February 3, 2023 10:29
@jdamanalo jdamanalo changed the title Close cpu_sup port on terminate os_mon: Close cpu_sup and memsup port on terminate Feb 3, 2023
@jdamanalo
Copy link
Contributor Author

Added closing of memsup too.

I considered closing cpu_sup using stop() as mentioned in the issue but I am unsure if it is actually working as intended (It does not seem to bubble up until the internal port). Also, it is unused by os_mon. Hence, I decided to just close it on the exit signal. Maybe it can be removed?

I also added more context in the open issue.

@rickard-green rickard-green added the testing currently being tested, tag is used by OTP internal CI label Feb 3, 2023
@rickard-green rickard-green modified the milestones: OTP-26.0-rc1, OTP-25.3 Feb 7, 2023
@rickard-green
Copy link
Contributor

Thanks for the PR. I've pushed a commit that

  • only send the quit message to the cpu_sup port server when we actually have a port server which prevents an error being logged in the case we do not have a port server
  • removed the receive of the never sent quit message in the cpu_sup measurement server
  • takes care of all places where the close message can be received in the memsup port handler process

Please squash this commit with your commit and force push it.

cpu_sup and memsup shutdown fixes
@jdamanalo
Copy link
Contributor Author

Squashed and rebased. Thanks for getting what I missed! 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants