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

Fix TZ cache exception blocking import #1705

Merged
merged 2 commits into from
Sep 30, 2023
Merged

Fix TZ cache exception blocking import #1705

merged 2 commits into from
Sep 30, 2023

Conversation

ValueRaider
Copy link
Collaborator

@ValueRaider ValueRaider commented Sep 27, 2023

Hopefully fix TZ cache exception blocking import #1700

The Singleton pattern makes unit testing this tricky, so can someone affected try branch? @rickturner2001

Also:

  • relocate cache init lock
  • add test for setTzCacheLocation()

Hopefully fix TZ cache exception blocking import. Also:
- relocate cache init lock
- add test for setTzCacheLocation()
@rickturner2001
Copy link
Contributor

rickturner2001 commented Sep 27, 2023

Just tested the code on a readonly docker container and it works just fine. I wanted to ask you about _TzCacheDummy

since the tz cache is registered by _get_ticker_tz in base.py, what made you want to initialize a cache with an instance of either _TzCache or _TzCache dummy instead of simply returning the tz value in the evnet that we don't have r/w access

what made you want to crate the Dummy class instead of doing something like this?

 def _get_ticker_tz(self, proxy, timeout):
        if self._tz is not None:
            return self._tz

        # assume this exists
        has_rw_permissions = utils.can_rw_to_fs()
        
        tz = None
        cache = None
        
        if has_rw_permissions:
            cache = utils.get_tz_cache()
            tz = cache.lookup(self.ticker)

            if tz and not utils.is_valid_timezone(tz):
                # Clear from cache and force re-fetch
                cache.store(self.ticker, None)
                tz = None

        if tz is None:
            tz = self._fetch_ticker_tz(proxy, timeout)

            if utils.is_valid_timezone(tz):
                # info fetch is relatively slow so cache timezone
                if has_rw_permissions:
                    cache.store(self.ticker, tz)
            else:
                tz = None

        self._tz = tz
        return tz

I think maybe the class makes it easier to test, but maybe you can shed some light on this. Thanks

@ValueRaider
Copy link
Collaborator Author

ValueRaider commented Sep 28, 2023

I didn't implement the sqlite architecture so couldn't answer. The person that did, seemed to know more about Python engineering than me so I didn't question design, but they aren't active anymore.

utils.can_rw_to_fs() seems to conflict with user changing cache location via utils.set_tz_cache_location. RW is a property of cache, not global.

@rickturner2001
Copy link
Contributor

is anyone still working on this?

@ValueRaider ValueRaider merged commit 9581b8b into dev Sep 30, 2023
@ValueRaider ValueRaider deleted the fix/tz-cache-init branch September 30, 2023 20:16
@ValueRaider
Copy link
Collaborator Author

I half forgot, half wasn't sure if you wanted changes.

@ValueRaider ValueRaider mentioned this pull request Oct 4, 2023
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.

2 participants