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

RFC: Safe LuaObject:ApplyAt API #5222

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

Conversation

laarmen
Copy link
Contributor

@laarmen laarmen commented Jun 27, 2021

This is an RFC API aiming to mitigate the safety problems caused by
Timer:CallAt as seen in plenty of issues, the latest of which is #5220.
The fundamental issue with the CallAt API is that the environment in
which the closure is called can be drastically different to the one in
which it was defined.

This new API aims to guarantee the caller that at least one object must
still be valid when calling the closure, by storing it out of band and
having the engine check its validity before calling the callback.

This would allow some simplification in existing correct callbacks, e.g.
in the assassination module, we could turn this

Timer:CallAt(mission.due, function () if mission.ship:exists() then mission.ship:Undock()

into

mission.ship:CallAt(mission.due, Ship.Undock)

Thoughts ?

This is an RFC API aiming to mitigate the safety problems caused by
Timer:CallAt as seen in plenty of issues, the latest of which is pioneerspacesim#5220.
The fundamental issue with the CallAt API is that the environment in
which the closure is called can be drastically different to the one in
which it was defined.

This new API aims to guarantee the caller that at least one object must
still be valid when calling the closure, by storing it out of band and
having the engine check its validity before calling the callback.

This would allow some simplification in existing correct callbacks, e.g.
in the assassination module, we could turn this

    Timer:CallAt(mission.due, function () if mission.ship:exists() then mission.ship:Undock()

into

    mission.ship:CallAt(mission.due, Ship.Undock)

Thoughts ?
@Gliese852
Copy link
Contributor

@laarmen what if the second argument is a method not from the 'Ship' table, but from another table (Space, for example)? Will the object be passed there as self?

@laarmen
Copy link
Contributor Author

laarmen commented Jun 27, 2021

I think so. My Lua memories are a bit fuzzy, but IIRC the whole "method" syntax is purely syntactical sugar.

When I wrote the example, I wasn't planning on it being reduced to directly passing a method, it was just a happy coincidence. You could also write it as follows.

mission.ship:CallAt(mission.due, function (s) s:Undock() end)

edit: fix syntax method in the code block.

@Gliese852
Copy link
Contributor

@laarmen
Also had a thought, maybe it can be done like this:

mission.ship:CallAt(mission.due, 'Undock')

imho in that case it will even be easy to serialize, although it only becomes usable for this object.

@laarmen
Copy link
Contributor Author

laarmen commented Jun 28, 2021

Serialization is out of scope. This API is meant to reduce the amount of boilerplate necessary for delayed calls in the Lua scripts. Such calls are not currently serialized (unless I'm missing something?).

I am vehemently against using strings instead of actual functions. See https://devcards.io/stringly-typed for more details on this. If you want to ensure you're using the correct method for your object, you can simply write

mission.ship:CallAt(mission.due, mission.ship.Undock)
-- or
mission.chip:CallAt(mission.due, function(s) s:Undock() end)

@Gliese852
Copy link
Contributor

@laarmen but lua is not strongly typed, and obj.field == obl['field'], if I'm not mistaken, anyway, I do not insist at all.
Yes pending jobs are not serialized. Although it seems we always want the task to start regardless of whether the game is saved or not.

@sturnclaw
Copy link
Member

I like this idea, though I think on its own it merely reduces to Timer.CallAt(mission.due, wrapperFn(mission.ship, Ship.Undock)); this isn't something that would require an entirely separate API, merely a little extra logic in a wrapper function to check if the object exists.

If we're going to go the route of adding additional API specifically for this type of pattern, I think we should look more closely at LuaEvent as well - it's a common pattern to write a function preamble in event handlers that tests if a specific argument a) still exists, and/or b) is a specific object or class of objects. This seems like a similar requirement to that proposed for Timer, and if an API pattern can be found that covers both use-cases it would certainly improve the code quality significantly.

@impaktor impaktor force-pushed the master branch 3 times, most recently from 876b6fc to b5bc47c Compare July 6, 2022 13:11
@impaktor impaktor force-pushed the master branch 2 times, most recently from 90e4530 to 6a3a044 Compare January 26, 2023 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants