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

Add supervisor:which_child/2 #8976

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Maria-12648430
Copy link
Contributor

Sometimes I have the need to obtain the Pid of a certain supervisor child. This often happens when the children form a work unit of cooperating processes, wherein one needs to talk to another.
To obtain such a Pid, currently I have to use which_children/1 to obtain a list of all children, then filter out the one I need, which is boilerplate code repeated over and over.

This PR adds a function which_child/2, which can be used to obtain the same information as which_children/1 does, but for a particular single child.

Copy link
Contributor

github-actions bot commented Oct 22, 2024

CT Test Results

    2 files     96 suites   1h 7m 17s ⏱️
2 167 tests 2 119 ✅ 48 💤 0 ❌
2 526 runs  2 476 ✅ 50 💤 0 ❌

Results for commit a720ea9.

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

@Maria-12648430 Maria-12648430 force-pushed the supervisor_which_child branch 2 times, most recently from f76cb7b to a9a9a78 Compare October 24, 2024 05:37
@u3s u3s added the team:PS Assigned to OTP team PS label Oct 25, 2024
@josevalim
Copy link
Contributor

I found myself needing a similar functional several times. 👍

@IngelaAndin
Copy link
Contributor

@Maria-12648430 I think it looks useful :) will bring it up on our PR-meeting today.

@IngelaAndin
Copy link
Contributor

@Maria-12648430 we decided this looks good would you please add test case too.

@Maria-12648430
Copy link
Contributor Author

@Maria-12648430 we decided this looks good would you please add test case too.

@IngelaAndin done 👍

@IngelaAndin IngelaAndin added the testing currently being tested, tag is used by OTP internal CI label Oct 31, 2024
Copy link
Contributor

@u3s u3s left a comment

Choose a reason for hiding this comment

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

This will be useful. Thanks!

which_children(Config) when is_list(Config) ->
{ok, SupPid} = start_link({ok, {#{}, []}}),

[] = supervisor:which_children(SupPid),
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the 99% same test code in 2 tests? maybe consider having a helper function for easier maintenance in future - so that redundant code is reduced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tl;dr, yes it is 😅

I can do it as you suggest and will readily yield to any such decision.

However, personally, I would argue that those tests test behaviors which are merely very similar, but not the same, and that this fact justifies two distinct fixed tests, even if they are also very similar to each other.

But you decide 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is a tradeoff. Sometimes it could be harder to understand the test case if you sort or loose the context.
It could of course be compensated with good variable naming's. I am not sure what I would think in this case without an example of how the helper should look.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was tired when doing review. My bad. After re-reading the code, I don't think it is worth changing.
Sorry for confusion. Pls, leave it as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@u3s np, will do 🤗

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said, I just augmented the which_children_simple_one_for_one test a little, to check that giving a non-pid to a simple_one_for_one supervisor results in {error, simple_one_for_one}.

@IngelaAndin IngelaAndin removed the request for review from dgud November 4, 2024 13:43
@IngelaAndin
Copy link
Contributor

Ok, just enabled this branch to be tested again, it gets automatically commented out when you make a new push. This unfortunately is not automatically mirrored by the label.

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

Successfully merging this pull request may close these issues.

4 participants