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 apply_* functions to the timermodule that accept anonymous functions #7649

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

juhlig
Copy link
Contributor

@juhlig juhlig commented Sep 12, 2023

This PR adds the functions apply_after/2, apply_interval/2 and apply_repeatedly/2 to the timer module that accept a 0-ary function as second argument, and the functions apply_after/3, apply_interval/3 and apply_repeatedly/3 that accept a n-ary function as second and a list of n arguments for this function as third arguments.

This is mainly a convenience feature, the same can be achieved via eg timer:apply_*(Time, erlang, apply, [Fun, Args]), and internally the new functions use this approach.
However, the new timer functions, when called, check the functions given to the timer:apply_*/2 functions to be of arity 0, and the lengths of the argument lists given to the timer:apply_*/3 functions to match the arities of the given functions.
Using timer:apply_*(Time, erlang, apply, [Fun, Args]) does no checking on being called, and mismatches are only discovered when the timer executes, without the process having called timer:apply_after/4 ever getting to know of it.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 12, 2023

CT Test Results

       2 files       91 suites   41m 23s ⏱️
1 995 tests 1 946 ✔️ 49 💤 0
2 299 runs  2 248 ✔️ 51 💤 0

Results for commit 4e97963.

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

juhlig commented Sep 12, 2023

Btw, I changed the tests for the related functions a little. Timer tests are slow by nature, and running the apply_* tests as they were for the new variants would have essentially tripled their run time, so instead I am starting three timers in parallel, one for each variant, and collect the expected result messages in parallel as well.

@juhlig
Copy link
Contributor Author

juhlig commented Sep 13, 2023

I added another commit to document caveats related to the use of self() in functions given to timer:apply_*.

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Sep 18, 2023
@juhlig
Copy link
Contributor Author

juhlig commented Sep 26, 2023

I just realized that apply_* timers can be tricky in the context of hot code upgrades, especially the apply_interval and apply_repeatedly variants, both with the old variants and the new ones introduced with this PR. As the new functions are mainly conveniences for timer:apply_*(Time, erlang, apply, [Fun, Args]), I will mostly stick to the old functions in my elaboration.

If the function denoted by the given MFArgs gets removed during a code upgrade, an apply_after timer it will fail, which can be reasonably expected. As the fail happens in a spawned function (vs in the spawn call), apply_interval and apply_repeatedly timers will continue trying to run the removed code, and failing.

Also, if the given MFArgs is erlang, apply, [Fun, Args] (which the new functions use internally), and Fun is an unqualified function in the same module (eg, fun myfunction/0), code changes will not be applied and the timer will keep executing the old code. Another code upgrade will cause this code to be dumped, and the timed spawns will result in badfun errors from then on.

I think this should at least be documented, and followed up by a mechanism to update running apply_* timers and/or a way to tell the timer server that interval-/repeatedly-timers should automatically stop if what they are trying to execute fails. WDYT? (@bjorng)

@michalmuskala
Copy link
Contributor

michalmuskala commented Sep 26, 2023

For the sake of code upgrades apply(fun module:name/Arity, [...]) is exactly the same as apply(module, name, [...]). So if you need to do hot upgrades, avoiding closures in there should be enough (local function references are always closures).
Using such functions has a lot of benefits compared to "bare" atom name tuples - they can be analysed by xref & dialyzer, located by code navigation tools (erlang_ls, ELP, ...). I'd consider adding support for them over the raw atom names a big win.

@juhlig
Copy link
Contributor Author

juhlig commented Sep 27, 2023

For the sake of code upgrades apply(fun module:name/Arity, [...]) is exactly the same as apply(module, name, [...]). So if you need to do hot upgrades, avoiding closures in there should be enough (local function references are always closures).

Yes, I understand that :)

My point is that, with timers involved there is a wide field of mistakes, unexpected things, or consequences that you were not aware of, which could pop up after a hot upgrade.

A timed function will be executed later, so any misbehavior like old code being executed or failing calls to code which went out of the code server would not be detected straight away, maybe not for a long time even. And as the calls are made in processes somewhat removed or dislocated from everything that is upgraded, this is not easy to detect even. Timed calls would just keep doing the old stuff, or fail to execute altogether, without anyone the wiser.

@Maria-12648430
Copy link
Contributor

What is the status of this PR, I wonder? I think it is a pretty useful addition, for the reasons @michalmuskala named and others.

@bjorng bjorng added the testing currently being tested, tag is used by OTP internal CI label Oct 6, 2023
@bjorng
Copy link
Contributor

bjorng commented Oct 6, 2023

I am generally in favour of this PR, but I might have some nits to pick regarding the documentation. I'll try to get to it early next week.

Copy link
Contributor

@bjorng bjorng left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, but I think that the functions that take both a fun and arguments are unnecessary and just clutter up the API with more functions than needed.

Comment on lines +77 to +78
<p>Evaluates <c>spawn(erlang, apply, [<anno>Function</anno>,
[]])</c> after <c><anno>Time</anno></c> milliseconds.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<p>Evaluates <c>spawn(erlang, apply, [<anno>Function</anno>,
[]])</c> after <c><anno>Time</anno></c> milliseconds.</p>
<p>Evaluates <c>spawn(<anno>Function</anno>)</c>
after <c><anno>Time</anno></c> milliseconds.</p>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, while that has the same outcome, spawn(Function) is not what it does. It does call spawn(erlang, apply, [Function, []]), as it currently says.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK.

Comment on lines 85 to 87
<name name="apply_after" arity="3" since=""/>
<fsummary>Spawn a process evaluating <c>erlang:apply(Function, Arguments)</c>
after a specified <c>Time</c>.</fsummary>
Copy link
Contributor

@bjorng bjorng Oct 9, 2023

Choose a reason for hiding this comment

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

I don't think we should have this function (nor the other ones that take both a fun and arguments). We don't use this style for the lists module or for spawn, so I don't think we should use it here.

(I see that timer:tc() since OTP 26.0 has variants that take both a fun and arguments. Being a family functions that is often use interactively, there is some justification for having extra convenience functions.)

<seemfa marker="#apply_after/4"><c>apply_after/4</c></seemfa>,
<seemfa marker="#apply_interval/4"><c>apply_interval/4</c></seemfa>, and
<seemfa marker="#apply_repeatedly/4"><c>apply_repeatedly/4</c></seemfa>
are executed in an own process, and therefore calls to <c>self()</c> in
Copy link
Contributor

Choose a reason for hiding this comment

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

"an own" feels strange to me:

Suggested change
are executed in an own process, and therefore calls to <c>self()</c> in
are executed in its own process, and therefore calls to <c>self()</c> in

or perhaps it would be better for clarity to write: "in a freshly-spawned process"

Copy link
Contributor

@bjorng bjorng left a comment

Choose a reason for hiding this comment

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

Thinking a little bit more (and re-reading the previous comments for this PR), I now realize that the functions taking both funs and arguments are indeed needed useful and needed if you want to support hot code loading and need to pass in an external fun.

@juhlig
Copy link
Contributor Author

juhlig commented Oct 12, 2023

Thanks for your comments and suggestions @bjorng, I'll see to it next week as I am currently on vacation :)

@bjorng bjorng removed the testing currently being tested, tag is used by OTP internal CI label Oct 16, 2023
@bjorng
Copy link
Contributor

bjorng commented Oct 16, 2023

Here is the ticket id to use for the since attributes: OTP-18808

@juhlig
Copy link
Contributor Author

juhlig commented Oct 16, 2023

@bjorng I added the ticket id to the since tags and used "a freshly-spawned process" instead of "an own process" as suggested above.

@bjorng
Copy link
Contributor

bjorng commented Oct 16, 2023

Thanks! Looks good. Please squash the commits.

@juhlig
Copy link
Contributor Author

juhlig commented Oct 16, 2023

Ok, done :)

@bjorng bjorng added the testing currently being tested, tag is used by OTP internal CI label Oct 16, 2023
@bjorng
Copy link
Contributor

bjorng commented Oct 16, 2023

Added to our daily builds.

@bjorng bjorng merged commit 1a3d53c into erlang:master Oct 18, 2023
15 checks passed
@bjorng
Copy link
Contributor

bjorng commented Oct 18, 2023

Thanks for your pull request.

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

5 participants