Issue #24 open

heaps of fixes

craigds
created an issue

We've been using livesettings for ages, but found a lot of things really buggy and broken.

I've started trying to fix the issues on my fork on github: https://github.com/craigds/django-livesettings/compare/6b0270030ebc56f466eca6e92cf43e85...master

You can pull it with hg-git or similar. The main changes are:

  • remove dependency keyedcache (since we found it to be really buggy, completely unnecessary since it provided zero features on top of django.core.cache, and caused lots of silent failures when our cache backend wasn't properly configured. It also logged a lot of things unnecessarily, filling up our syslog with a line for every setting on every page render.)

  • the dodgy ConfigurationSettings 'singleton' (which was really not a singleton at all) has been deprecated. Instead there's a module level instance of this class (from livesettings import configuration_settings ...) The singleton thing still works for now, but should be removed in a future release.

  • replaced a lot of catchall except: blocks; we found they were masking errors, making things hard to debug.

There's a lot of changes in this; I realise this will take some reviewing, but please don't reject it outright as it is a huge improvement to the reliability and usability of django-livesettings. It's also backwards compatible for the most part. I'm open to removing backwards incompatibilities if you find some.

Comments (10)

  1. Hynek Cernoch

    > remove dependency keyedcache (since we found it to be really buggy...)

    Can you be more specific? Yes, it is not nice and I also suspect that it can cause some problems, but keyedcache has no important open issues, except trivial ones, which I have fixed and the patches are waiting for accepting now. Can you report bugs? Not only deprecated features, but reported reproducible serious bugs are important to give for such big changes.

    > Keyedcache is completely unnecessary since it provided zero features on top of django.core.cache

    Keyedcache adds selective deleting by key mask, some debug logging and views for cache administration. It is not completely bad, but it does not reflect development of Django in the last 2 years. Similar functialities can be now implemented as a specialized cache backend which would be a superstructure over the normal Django cache and can be replaced by it. Keyedcache is useful probably for single process development or for single process multithread server, because the list of cached keys is stored in a global variable.

    > The singleton thing still works for now, but should be removed in a future release.

    It should cease to work, depending on future release of what? You or somethin inevitable? Satchmo depends directly on ConfigurationSettings now, which is not much transparent.

    > Instead there's a module level instance of this class.

    Packages livesettings + keyedcache + threaded_multihost were intended to work with more sites little different with the same settings.py and with more threads. I never tried it.

    > replaced a lot of catchall `except:`

    Good, but it would take some time to find all important exception types.

    > I'm open to removing backwards incompatibilities if you find some.

    The CACHE_PREFIX is not supported, which would be a complication for upgrading existing sites, especially where are more sites using the same memcached.

    --

    The only serious bug of livesettings known to me #21 is not probably related to anything from the aforementioned.

    I like the simplification and clarity, but I dislike that is not easy to prove the compatibility. Livesetting were originally a part of a big package Satchmo and there will have to be now a combined direct access to cache and via keyedcache.

    I think that the best would be to create a new branch e.g. 2.0 and some future fixes to apply temporarily to both branches. Default branch should be some time the original 1.x and the user could switch it arbitrarily.

    We'll see more when Chris comes back from vacation.

  2. Hynek Cernoch

    I think, that changes 232254fe, 0e1024f1, 1490da26 can be accepted without any discussion. I add them soon to my livesettings repository with my changes I am working now and I send a pull request. Some my typos fixed in other changesets I fixed independent even before you.

    I think that your courageous solution is very helpful. (Though I would have dozens of small particular objections.) If something really works (or appears only that it works?) is better than a great new idea.

    Please write the bugs of existing livesettings, you know about !! Your report is too general about some changes. Be more specific than "buggy and broken and deprecated".

    It is easier to fix something for me myself, but not so easy to fix it for all existing installations not known me including multithreading etc. Is your solution better also with an application which _must_ concurrently use keyedcache? (Satchmo)

    > It also logged a lot of things unnecessarily, filling up our syslog...

    Yes, it is usual to suppress it by configuring it in settings.py:
    import logging; logging.getLogger('keyedcache').setLevel(logging.INFO)

    I write it to the documentation. I have enabled the level DEBUG intentionally for keyedcache several times.

  3. craigds reporter

    Thanks Hynek :)

    Apologies for my terseness re keyedcache. I didn't realise it did any extra stuff - we don't use any of its invalidation stuff or views, we just want to have memcached settings, and it seemed to just be getting in the way. I just assumed it was a leftover from an old version of satchmo before django's caching mechanisms were well developed.

    Given that keyedcache is of benefit to some users, but an inconvenience to others, can I suggest a settings.LIVESETTINGS_USE_KEYEDCACHE flag to choose whether it is used or not? I will revert my keyedcache removal in favour of a settings flag, if that's more likely to make it upstream.

    It should cease to work, depending on future release of what?

    I was just implying that ConfigurationSettings() as a singleton should become not-recommended and eventually unsupported in a future release of livesettings, once dependent libraries are using the new module-level instance, but up to whoever's in charge... Singletons ... basically remind me of Java, but I don't have a problem with them per se. It's just that Python doesn't really need them since you can use module level instances. See http://stackoverflow.com/questions/31875/is-there-a-simple-elegant-way-to-define-singletons-in-python

    My main objection is that technically the ConfigurationSettings singleton isn't even a singleton, since all the instances are different instances and they delegate to an anonymous shared inner object. I feel if we're going to have a singleton it should at least be a real singleton (i.e. see the way my ConfigurationSettings singleton is implemented in my livesettings fork) But better still is to use a module level instance.

    For example, an unintended side effect of the weird way that the 'singleton' is currently implemented:

    >>> a = ConfigurationSettings()
    >>> b = ConfigurationSettings()
    >>> a == b
    False
    

    The other main thing my changes did was replace the catchall try/except blocks with more selective ones. I left one in because I wasn't sure what exactly it was supposed to catch. For the rest I've added sensible exception classes - feel free to recommend others but I'm fairly sure these are okay? The idea here is to specifically *not* catch anything that's not expected in a 'normal' situation. Catching and logging is not nearly as good as not catching at all, since if you don't catch, the behaviour is customizable by the caller.

    I don't really want to maintain my own fork so I'll happily rearrange it to make it more pullable if you let me know about the above.

    Regards Craig

  4. Hynek Cernoch

    I see that some existing bugs can be then easier fixed, though you did not reported no new.
    Keyedcache can be good for some debugging, but for production it would be better to use directly django cache.

    Pullable?
    Nobody can beforehand say: do this so and so and it is enough to be your solution pullable.
    We (probably you and I) should do first:

    • A little better documentation for livesettings:
      - What is expected to be stable interface for external applications.
      - What could be used by other people in good faith in the past or by the Bruce Kroeze (author) in his deployments, even if we did not documented it finally.
      - What was maybe intended to be backward compatible interface even if this does not look meaningful today.
    • Write some missing tests for important parts (so that Chris can relative easy verify that the new is good)
      maybe write more complicated test than Django builtin tests permits.
      - Test which verify real syncdb on a database with transaction support,
      - test with real cache which is not flushed on process startup,
      - test multithreading with inserted small delays in some problematic places to be more probable to catch the problems.

    I have several kilobytes of new tests now.

    Singleton
    You are right. (I have read the same text on stackoverflow when I was first modifying livesettings.) The name beginning with an underscore for a module variable is the best possible singleton in Python. (The only problem is that a module can be imported more times under different names e.g. import project_directory.my_application versus import my_application. It is unlikely that this would be used for an installed the package like livesetting, but it would be probably the same problem for the solution singleton like.)

    The following construction in ConfigurationSettings manager is not safe in the multithread environment without locking around. :

        if xyz == None:
            # expect some delay here especially on multithread single core system
            # other thread do the same in middletime, reads values from from db, stores to cache
            xyz = Something()  # instance    # maybe overwrite
            # expect delay - other thread now reads its values and finds nothing
    

    This can be the reason of writing SomeComplicatedSingletonLikeClass().method(...) istead of something more efficient.

    This is also unsecure (imho):

        if not groupkey in self.settings:
            # expect some delay here
            self.settings[groupkey] = g
    

    Can you test you solution on a multithread webserver? Do you know other than deprecated mod_python?

    Very unusual is comparing groups by its translated name: (What if the language is switched? Are compared strings or proxies?)

        def __cmp__(self, other):
            return cmp((self.ordering, self.name), (other.ordering, other.name))
    

    General excepts
    Yes, even if we are careful, it creates probably some unexpected issues, but it helps hopefully to find easier some old reported unclear random bugs again.

    It is not possible to catch exactly database errors on some Django versions and db engines because they are not descendants of DatabaseError, but somemodule.DatabaseError. (I think some Django 1.2 and mysql or sqlite3)
    solution is:

        except Exception, e:  # do not catch exit or Ctrl-C
            if str(e).find('DatabaseError') >= 0:
    

    Some time we must support installation of the classic version somehow.

  5. craigds reporter

    Yes, I have noticed several thread safety issues with livesettings already. However I don't believe my fixes have made the situation any worse... Do you disagree?

    I have just spent some time cleaning up the changes on my fork. I've split the larger commits up into more descriptive chunks. I've also added the settings.LIVESETTINGS_USE_KEYEDCACHE thing I proposed above.

  6. Hynek Cernoch

    I am now unexpectedly satisfied with this way.

    I think removal of bastion and the citadel :-) ...
    ... is the best change. I had many problems with writing tests because I needed to go inside. It succeeded finally, not nice but succeeded: For testing http post by test client in livesetting is necessary to remove old values, for testing an application which uses livesettings it is necessary to restore them unchanged, because they are imported only once at startup and not expected to by changed by other test.

    Please read comment which I wrote to some lines into your changesets code directly in Github. I read it yesterday slowly with a fast complete good understanding and with searching what is beeing used in other parts of code including Satchmo. Most of them are threading related.

    Some notes to the public interface:

    • I looks that what public was originally intended, is only in the namespace "from livesettings import *". Important for users understanding is the attribute `all`.
    • A big question is the constant "NOTSET = object()" for uninitialized values. It should be internal but it sometimes escapes and DurationValue looks sometimes like "<object object at 0x7f553f8b3130>" in the web form :-)) It can be necessary the instance be known for someone because "object()!=object()"
    • I am not sure how much necessary is the configuration_settings variable in other modules. Maybe only " configuration_settings.groups()" for enumerating all groups, which is not typical in the external. Do you think that configuration_settings looks evenso open/closed by naming conventions ("_...") how it is helpful? (We should not publish any interface sensitive to users thread safety bugs.)
  7. Hynek Cernoch
    • changed status to open

    1) I appreciate your github change Remove longsettings table since it is pointless

    Before South it was easier to add a table than to change a column in a shared project.

    Pros:

    • Every value which is not different from default value is searched in both tables and requires two queries.
    • More advanced error handling can be written.
    • It would be advantageous to read all livesettings values from the database to the cache at startup (first access to any livesettings value) by one query and store them in the cache including caching values which does not exist in the database. It would be easier to log eventual problems in initial phases of Django. (All other would be the same, only pre-filling the cache would be added.)

    Cons:

    • Data migration should be written to move data from longsettings table to settings table before removing the old.

    2) Very good is also add settings.LIVESETTINGS_USE_KEYEDCACHE to disable/enable..
    because it helps to simplify complicated problems and also to compare benefits of two-level keyedcache for actual configuration of the server.

    Caching in Django is improving constantly. Very interesting is KEY_PREFIX and KEY_FUNCTION support in Django 1.3 and "Using a custom cache backend" which all together provides high modularity standard of caching.

  8. Bruce Kroeze repo owner

    Hi, this is Bruce, the original developer of this app.

    What is the status of this merge?

    Hynek?

    I think many of the arguments are valid. The code, or a lot of it needs to be merged. As the owner of this repo, I'd be happy to do so. What's the current thought about it?

    I do agree about keyedcache not beeing needed anymore. It was a reaction to the silly old caching code that Django is far past at this point.

  9. Hynek Cernoch

    Hi, Bruce, I am pleased to meet you.
    I'll be glad if you decide what to use and how you prefer if. I am often too cautious and Craigds was probably tired of my comments. Without him I had little motivation.

  10. Log in to comment