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

only_once will be called multiple times #22

Closed
stub42 opened this Issue Nov 9, 2015 · 10 comments

Comments

Projects
None yet
4 participants
@stub42
Copy link
Collaborator

stub42 commented Nov 9, 2015

The guard used to stop only_once decorated code from running more than once includes the code line number in its key. The method will be run again if it is moved around the source file, or into a different source file for that matter. Which is fine, as I can argue that running only_once decorated methods once after every upgrade-charm hook is a good thing.

I think this should be explicit, rather than random, and the was_invoked/marked_invoked caches cleared at the beginning of an upgrade-charm hook. If people do need a only_once_really decorator, the developer would need to provide a key rather than a volatile one generated for them.

@galgalesh

This comment has been minimized.

Copy link
Collaborator

galgalesh commented Jan 28, 2016

I'd argue against making this explicit. I think the main use-case for only-once is a handler that isn't idempotent, therefore it should never be allowed to run again, even during an upgrade-charm hook.

@stub42

This comment has been minimized.

Copy link
Collaborator

stub42 commented Jun 22, 2016

We have the choice between:

  • leaving things as they are and documenting the current behaviour
  • having only_once reset on upgrade-charm so they are always rerun on upgrade-charm rather than sometimes rerun on upgrade-charm
  • requiring or optionally allowing the developer to add an explicit key
  • deprecating only_once.
@galgalesh

This comment has been minimized.

Copy link
Collaborator

galgalesh commented Jun 23, 2016

My vote goes to deprecation. The main use-case for only-once is a handler that isn't idempotent, such as formatting HDFS.

  • Non-idempotent handlers are already an anti-pattern since a handler might run multiple times during debugging.
  • The only_once functionality can be simulated by using states if you really need it.

Also, removing stuff just feels good :), removing complexity and such...

@galgalesh

This comment has been minimized.

Copy link
Collaborator

galgalesh commented Feb 14, 2017

@stub42 , @johnsca ,

How do we go about deprecating this? Is there any way for us to give warnings at charm build time? Such functionality will also be useful for the 2.0 switch.

@ajkavanagh

This comment has been minimized.

Copy link
Collaborator

ajkavanagh commented Feb 15, 2017

Over the years, I've written deprecate decorators of the form:

import functools


def deprecate(warning, logger=None):

    def wrap(f):
        _call_count = 0

        @functools.wraps(f)
        def wrapped_f(*args, **kwargs):
            nonlocal _call_count
            if not _call_count:
                # log or print deprecation here
                print(warning)
                _call_count = 1
            return f(*args, **kwargs)
        return wrapped_f
    return wrap


@deprecate("Don't use this")
def deprecated_function():
    # do something
    print("deprecated_function")


@deprecate("This one either")
def f2():
    print("f2 away")


deprecated_function()
deprecated_function()
f2()
deprecated_function()
f2()
f2()

with the corresponding output:

python3 test-deprecate.py
Don't use this
deprecated_function
deprecated_function
This one either
f2 away
deprecated_function
f2 away
f2 away

This has the advantage where the deprecated function will only log once, regardless of how many calls there are. It's then fairly easy to add a date to be removed and just grep the source to see when to remove it.

@marcoceppi

This comment has been minimized.

Copy link
Member

marcoceppi commented Feb 15, 2017

@ajkavanagh

This comment has been minimized.

Copy link
Collaborator

ajkavanagh commented Feb 15, 2017

If it was only wanted at build time (which, on reflection, might be more useful), then either a simple-ish regex of the source code would reveal if there were any only_once decorators.

An alternative solution might be to mock the only_once decorator and try to walk the built charm and import /reload all the modules which would spit out if it was being used. We do something similar in charms.openstack to verify which reactive decorators we have used during unit testing.

@galgalesh

This comment has been minimized.

Copy link
Collaborator

galgalesh commented Feb 15, 2017

@marcoceppi

This comment has been minimized.

Copy link
Member

marcoceppi commented Feb 15, 2017

regular experssion feels wonky to me, but if it can be done well and clean I'm not going to fuss either way.

@galgalesh galgalesh referenced this issue Nov 6, 2017

Merged

More doc fixes #140

@stub42

This comment has been minimized.

Copy link
Collaborator

stub42 commented Sep 17, 2018

only_once has been deprecated in favor of the common @when_not('a') [...] set_flag('a') pattern

@stub42 stub42 closed this Sep 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment