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

ets: add ability to suppress heir/gift data #7970

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

Conversation

Maria-12648430
Copy link
Contributor

The current implementation of the heir option as given to ets:new/2 and ets:setopts/2 requires that a term is given as HeirData in a {heir, HeirPid, HeirData} 3-tuple, which will be sent in an {'ETS-TRANSFER', ...} message when the owner process dies and ownership passes to the heir process.

This is not always feasible, for example when the heir process is a supervisor which passes the table to a child process on (re-)starts. If the child process dies, the supervisor will receive an 'ETS-TRANSFER' message, which it can't do anything with, and will just log it as an unexpected message.

This PR changes the behavior of ets:new/2 and ets:setopts/2 such that the heir option to also accepts a 2-tuple of the form {heir, HeirPid}, ie omitting the HeirData. If the owner process dies, table ownership transfers to the heir process quietly.

This PR also adds a new function give_away/2, in order to do explicit transfer of table ownership quietly. This will probably be of somewhat lesser use, and is only added for the sake of symmetry/completeness.

Copy link
Contributor

github-actions bot commented Dec 18, 2023

CT Test Results

    4 files    223 suites   1h 25m 54s ⏱️
3 473 tests 3 379 ✅  94 💤 0 ❌
4 895 runs  4 776 ✅ 119 💤 0 ❌

Results for commit 637da61.

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

@juhlig
Copy link
Contributor

juhlig commented Dec 19, 2023

Rebased because of merge conflicts.

@juhlig
Copy link
Contributor

juhlig commented Dec 19, 2023

The failing test seems to be unrelated to the changes made in this PR.

@sverker sverker self-assigned this Dec 19, 2023
@max-au
Copy link
Contributor

max-au commented Jan 6, 2024

While I don't mind the change in general, I think the justification is questionable. I think it promotes an anti-pattern I observed several times in the past:

This is not always feasible, for example when the heir process is a supervisor which passes the table to a child process on (re-)starts. If the child process dies, the supervisor will receive an 'ETS-TRANSFER' message, which it can't do anything with, and will just log it as an unexpected message.

It stands out to me, that if you want the supervisor to own the ETS table, you should do that from the very beginning. That is, create the table in the init callback, and pass the table reference to children that need it.

One should think of an ETS table as a part of the (owner) process state. If your process crashes, but you want to save (some?) of its state, it sounds alarming. When the process restarts but it keeps the state (by giving it away to an unsuspecting supervisor), chances are, another crash is coming up. Until finally the supervisor decides to give up, and crashes too (wiping out the ETS table, and eventually restarting a larger tree).

If that's the behaviour you're after, you should not need this PR, as you could just create the ETS table in the supervisor (and keep ownership by the supervisor).

@juhlig
Copy link
Contributor

juhlig commented Jan 7, 2024

@max-au hm, it sounds like you grabbed the wrong end of the stick somehow 😅 I'll elaborate on it later when I get my hands on a proper computer, I'm on my mobile on a shaky train right now 😵‍💫

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Jan 8, 2024
@juhlig
Copy link
Contributor

juhlig commented Jan 9, 2024

@max-au sorry for the delay in answering 😅


For one, this PR doesn't add anything really new. It does not enable (or disable, for that matter) anything that couldn't already be done now, bad and good alike.


For another, don't pay too much attention to the introduction of give_away/2, that is, the ability to give away a table to another process without implying the sending of an 'ETS-TRANSFER' message. This is only for completeness' sake, the actual objective of this PR is to let an appointed heir process inherit a table from a dying process without being sent such a message.


That said, the idea is to let a supervisor create a table in its init function, and appoint itself as that table's heir. When it then starts a child that is supposed to somehow work with the table, it will give away this table to that process. When the child dies, the supervisor gets back the table (this is the point where, as of now, the supervisor would receive an 'ETS-TRANSFER' message, which it can't do anything with and will just log it as an unexpected message; with the changes in this PR, it is possible to not provide any HeirData in the heir tuple, effectively suppressing the unexpected message), and will give it away again when restarting the crashed process. As such, the supervisor is not "unsuspecting", as it created the table and made itself the heir, so it should also expect to get back the table at some point in case the child crashes.

Instead of giving the table away, the supervisor could in fact also stay its owner, but then there would be no choice but to make the table public.


You point out that this could be used in an attempt to save some of the owner process' state, and I agree that this is bad design, an anti-pattern.

However, in modern applications, a supervision (sub-)tree often represents a work unit of several cooperating processes, instead of single worker processes (this is also what gave rise to EEP 56). Viewed from this angle, such a table could be seen as the state of the work unit, instead of as the state of a single process.

Depending on the task and subsequently the structure and (in-)dependence of the constituting child processes, it might be possible to restart one or another of them if they crash, without the need to crash the entire work unit.

I agree that misuse is possible, yes, but it is already possible right now, at the expense of some logging noise.
As always, careful designing is required, in the sense that the table in question should not contain any of or be treated as the state of the process it is given to, but as a state of the work unit the process is a part of.

@sverker
Copy link
Contributor

sverker commented Jan 10, 2024

The purpose of the 'ETS-TRANSFER' message was to avoid the risk of table leakage due to the receiver not being aware. Without it you must make sure to make the receiver aware some other way. For an heir like a supervisor I can see that's not too difficult to achieve with links or monitors. But the give_away is more difficult to make safe without an automatic and atomically sent message, I think. I understand the urge for symmetry/completeness but maybe we should skip it due to its unsafeness.

@juhlig
Copy link
Contributor

juhlig commented Jan 11, 2024

@sverker ok, it is probably a bad idea to create a new button only to label it "Don't push!" 😅 So if this is your final verdict, I'll remove this part again.

@juhlig
Copy link
Contributor

juhlig commented Jan 12, 2024

@sverker last commit removes give_away/2 again.

@max-au
Copy link
Contributor

max-au commented Jan 21, 2024

It does not enable (or disable, for that matter) anything that couldn't already be done now, bad and good alike.

That I understand.

I agree that misuse is possible, yes, but it is already possible right now, at the expense of some logging noise.

And this is exactly the kind of "soft friction" I like. One is not prohibited from leveraging this use-case. But there is a nagging indicator "you might be holding it wrong". :-)
When there is no such indicator, it's fair to assume that this design is endorsed by Erlang/OTP.

To give some more background, at a previous place there was quite an amount of code using heir as a way to save ETS tables from the programmer's error (and a recommendation "always use the supervisor as a heir so you don't lose your tables, and other processes reading your table are unaffected by your crash-looping process). To a degree that we even had a custom patch that suppressed logging of ETS-TRANSFER message by the supervisor... now, bitten by that, I'm somewhat opposed to changes that may inadvertently promote technique of "assigning some heir just to keep ETS table exist while my process is crash-looping".

Having said that, I'll repeat that I don't mind merging this PR. There is some value in it, even though it may introduce some "happy debugging, folks" episodes later.

Co-authored-by: Maria Scott <maria-12648430@hnc-agency.org>
@juhlig
Copy link
Contributor

juhlig commented Feb 13, 2024

Last commit solves merge conflicts caused by the merge of #8026.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants