Feature Request: @time_trigger('shutdown')

[dlashua]

Methods decorated with @time_trigger('shutdown') should be run:

1. When Home Assistant is shutting down
2. Any time a script/app is reloaded (either all shutdown methods, or those in a specific global context if reload is called with one)
3. When implemented, if Feature Request: Automatic reloading for existing scripts? #74 detects a deleted or commented out file, it should run shutdown for that global context before unloading the script.
4. Basically, any time the methods in that context will either not be loaded again or if the next step is to run startup again.

Use Case:

This is mostly for cleanup tasks.

If there is a script that, say, counts the number of lights on and reports it in pyscript.lights_on_count, the shutdown function could send "unavailable" as the state letting the rest of the system know that this sensor is no longer being updated.

Any method that doesn't execute and complete immediately could be cleaned up with this. For instance:

@time_trigger('startup')
def counter():
  task.unique('counter')
  cnt = 0
  while True:
    log.info(cnt)
    task.sleep(1)

Even if this method is commented out and reload is called, the script continues to count. A shutdown like this:

@time_trigger('shutdown')
def stop_counter():
  task.unique('counter')
  log.info('stopping counter')

would be able to stop the counter if this was desired.

If an async connection is made to another server, that connection could be closed.

[craigbarratt]

Sounds like a good idea. But I need to think about whether there are any race conditions with triggering functions just as their global contexts and trigger tasks are being destroyed.

[dlashua]

Yeah... there are lots of things to consider and it can get messy. It also digs around in the decorator code of pyscript which is why I haven't attempted a PR.

Another thing to consider is a poorly written shutdown function. What if there is an exception during shutdown? What if shutdown has an endless while loop? I think a timeout needs to implemented and the task should be cancelled if it takes longer. If you want to get really crazy @time_trigger('shutdown(30)') could mean, this shutdown function needs 30 seconds to complete.

I think safest way would be to wait for all shutdown tasks to complete (or be cancelled), then proceed as it does now by destroying global contexts and trigger tasks, and then performing the reload and the usual startup tasks. This means shutdown tasks can make a reload much slower, which is expected. But, because it's now possibly slower, we need to consider what happens if we get a ANOTHER reload request (by service call, or automatically if #74 is implemented) while shutdown/startup is still running.

If it helps any, this very basic test seems to work. I can comment these lines out of the file and reload and the counter stops.

@time_trigger('startup')
def counter():
    task.unique('counter')
    cnt = 0
    while True:
        cnt = cnt + 1
        log.info(cnt)
        task.sleep(1)

@event_trigger('call_service', 'domain == "pyscript" and service == "reload" and service_data["global_ctx"] == "file.test"')
def reload_called():
    log.info('reload')
    task.unique('counter')

[dlashua]

This ALMOST works:

@time_trigger('startup')
def byebye():
    task.unique('shutdown')
    mytask = asyncio.current_task()
    mytask.add_done_callback(log.info)
    while True:
        task.sleep(1)
        log.info('waiting for shutdown')

The issue is that add_done_callback requires a non-async callable. And EvalFuncVarAstCtx produces an async callable. log.info specifically works because it is not async, but any user defined method would actually be a EvalFuncVarAstCtx.

[craigbarratt]

I just pushed an initial implementation. I haven't added tests or docs yet so it's only minimally tested.

One issue is that kwargs for startup is:

{'trigger_type': 'time', 'trigger_time': None}

while kwargs for shutdown is:

{'trigger_type': 'shutdown', 'trigger_time': None}

For consistency it would best to change trigger_type to startup, but that's a breaking change.

[dlashua]

excellent. I'll test this this morning as well.

As far as breaking changes go, I don't think that one will affect too many people. I use time_trigger and state_trigger together on MANY functions and it won't break any of mine. There's not an easy way to slowly deprecate it either, since it's an output. You could announce in the next release that it will be changing so people can alter their code to accept either. Then in the release after that make the change and people can delete the "old" code.

[dlashua]

All of my simple tests have worked perfectly. This includes shutdown functions in scripts and apps, as well as multiple @time_trigger('shutdown') functions in the same file.

I even tested putting a shutdown trigger in a module (though I have no real use for this currently) and it worked perfectly. The shutdown function ran every time the module was reloaded. But, if no scripts were actually using the module, then, since it was never actually loaded, the shutdown function didn't run.

I have several apps that have workaround code to deal with needing but not having a shutdown trigger. I'll modify those later today. But, based on these tests, it will work as expected.

[craigbarratt]

It looks like the two choices are to either change the startup kwargs to:

{'trigger_type': 'startup', 'trigger_time': None}

which would be a (minor) breaking change, or to add another parameter to both, eg:

{'trigger_type': 'time', 'sub_trigger': 'startup', 'trigger_time': None}
{'trigger_type': 'time', 'sub_trigger': 'shutdown', 'trigger_time': None}

The former is cleaner and better, but not backward compatible. I agree it's unlikely to trip up many people, since I would assume they just check trigger_time is None to detect startup, in the rare cases there is a startup function that also triggers at other times.

Preference?

(Actually, with the benefit of hindsight, startup and shutdown should really be event triggers, not time triggers. Oh well.)

[dlashua]

pyscript is still young. I think we go for cleaner and better. and if you think event triggers make more sense (I can agree with arguments for either) then you may as well do that now too so it's all in the same "breaking change".

The only issue I see with using event_trigger, is right now, @event_trigger('EVENT_NAME_HERE) means that we can expect to see a Home Assistant event of "EVENT_NAME_HERE". Introducing a non-home-assistant event isn't likely a breaking change on its own, but it's a departure from current expectations of the decorator. @time_trigger is currently only operating on pyscript specific strings (startup, once(blah,blah), period(blah,blah), etc), and both "startup" and "shutdown" are "times" in the same way that "sunset" is a time (always changing, but still, a specific time "event").

pyscript COULD actually fire events for all startup and shutdown actions (which I'm sure someone would find useful), but, even then, the startup and shutdowns are specific to each global_ctx due to all of the smart reloading. So, that event should have global_ctx=whatever as a data element and, strictly speaking, the event trigger should also specify that same global_ctx as an expression (@event_trigger('pyscript.startup', f"global_ctx == '{__name__}'")), which is a lot messier than @time_trigger('startup').

[dlashua]

One more thing to add, since this is a time_trigger, having a trigger type of time really makes sense, which leads to a slight preference for the latter (with sub_trigger).

A third option, which would still be "breaking" but maybe cleanest of all would be:

{'trigger_type': 'time', 'trigger_time': 'startup'}
{'trigger_type': 'time', 'trigger_time': 'shutdown'}

[craigbarratt]

I like your last one the best, so that's what I just pushed, together with docs and tests.