#233 Added interface for performing resource clean up and updated tests #236

Quinny · 2017-05-23T23:42:51Z

No description provided.

Issue #233: Fixed resource clean-up in cached decorator.

codecov-io · 2017-05-23T23:46:19Z

Codecov Report

Merging #236 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #236      +/-   ##
==========================================
+ Coverage      99%   99.03%   +0.03%     
==========================================
  Files          12       12              
  Lines         705      727      +22     
  Branches       76       77       +1     
==========================================
+ Hits          698      720      +22     
  Misses          7        7

Impacted Files	Coverage Δ
aiocache/base.py	`100% <100%> (ø)`	⬆️
aiocache/decorators.py	`100% <100%> (ø)`	⬆️
aiocache/backends/memcached.py	`100% <100%> (ø)`	⬆️
aiocache/backends/memory.py	`100% <100%> (ø)`	⬆️
aiocache/backends/redis.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f2358d...c034795. Read the comment docs.

argaen

Thanks for your work, looks good! One extra comment, could you try to not decrease the coverage? :). You can check what you are missing with make cov

argaen · 2017-05-25T07:01:48Z

aiocache/backends/redis.py

@@ -121,6 +121,13 @@ def __init__(
            kwargs["encoding"] = encoding
        return await getattr(_conn, command)(*args, **kwargs)

+    @conn
+    async def _close(self, *args, _conn=None, **kwargs):


Close doesn't need the _conn to be propagated

argaen · 2017-05-25T07:03:01Z

aiocache/base.py

+    @API.register
+    @API.aiocache_enabled()
+    @API.timeout
+    @API.plugins


Can you remove register, aiocache_enabled and plugins decorators? Those decorators are for cache specific commands

argaen · 2017-05-25T07:04:48Z

aiocache/decorators.py

@@ -55,7 +55,9 @@ def cached_decorator(func):

            try:
                if await cache_instance.exists(cache_key):
-                    return await cache_instance.get(cache_key)
+                    result = await cache_instance.get(cache_key)
+                    await cache_instance.close()


Hmmmm we don't need to block the user until the pool is closed. asyncio.ensure_future would be sufficient here

I agree, but won't that bring back the warnings? The future created must still be awaited at some point.

I think it would be best to re-use the same connection within the decorator, hence my original proposal. Opening and closing the connection on each function call seems superfluous to me.

I agree with you on reusing the connection for the decorators, this is something I have in mind because there are a couple of edge cases.

Regardless of reusing or not the connection, its not worth it to delay the response to the user waiting for the close to pool. I prefer having a message saying Task X wasn't executed rather than unneeded delayed responses. We can think how to close those pending connections later :)

I see where you are coming from, but I don't think this "delay" is as big of a deal as you are making it out to be. Since the code is using asyncio it is not as if the program will be idle whilst closing the connection, the event loop will context switch to another task.

I have made the changes and the error messages have now returned. I would advise either reverting back to how I originally wrote it (awaiting the close), or reusing the same cache instance within a decorator. Leaving these connections and co-routines dangling to spew errors is more of a code smell then either of those options in my opinion. Ultimately though, it is up to you.

Yeah, I'm not talking about this kind of speed but speed for a single call. In order for the function to return, it will have to wait for the pool to close (and the loop to reschedule it) when it really doesn't have to to retrieve the result.

In an API with high load, this can lead to unnecessary delayed calls. Imagine the following case:

New call arrives to the decorated function F

Other 1000 async calls arrive (no need to be on the same function, any async will do)

F gets the loop and executes until arriving to the await close. (I know there are more breaks in the middle but for the sake of simplicity lets imagine we are there already). Because of finding the await it is rescheduled to the queue of tasks to execute (as the last one because its round robin).

Until all 1000 async tasks aren't processed (i.e. find their first break), the loop won't pick again F and it will pick it just to return something it could have returned already.

Also there can be side effects when trying to close the connection (https://github.com/aio-libs/aioredis/blob/master/aioredis/connection.py#L315) that raise exceptions.

Because all of that I prefer to use ensure_future. I agree with you that its a code smell and we won't be freeing all the resources but I prefer it that way. I have in mind a way to solve these situations, when I mature the idea a bit more I'll ping you to validate it if that's okay.

I hope this convinces you :)

Not entirely, I guess we will agree to disagree haha. I have made the changes to use ensure future anyways.

argaen · 2017-05-25T07:04:59Z

aiocache/decorators.py

@@ -67,6 +69,7 @@ def cached_decorator(func):
            except Exception:
                logger.exception("Unexpected error with %s", cache_instance)

+            await cache_instance.close()


argaen · 2017-05-25T07:05:03Z

aiocache/decorators.py

@@ -145,6 +148,7 @@ def multi_cached_decorator(func):
                    except Exception:
                        logger.exception("Unexpected error with %s", cache_instance)

+            await cache_instance.close()


argaen · 2017-05-25T07:06:32Z

examples/cached_alias_config.py

@@ -33,6 +33,7 @@
    assert await cache.get("key") == "value"
    assert isinstance(cache, SimpleMemoryCache)
    assert isinstance(cache.serializer, DefaultSerializer)
+    await cache.close()


This is not ideal. caches.get('default' returns a singleton identified by 'default'. User may want to use this cache in another function.

Global aliased caches should be closed when the program exits

The user can choose to close this cache whenever they want. This is just an example usage, no?

yup, its just for the sake of "good" example. Its not ideal to close a singleton cache in the middle of a cache :)

Fair enough, I've moved the closing of the cache into the body of test_alias

Quinny · 2017-05-25T16:17:53Z

All checks now passing

…tion in decorators

argaen · 2017-05-25T20:20:28Z

Thanks for your contribution. As said I'll ping you when I have the solution for cleaning up the rest of resources :)

Quinny added 9 commits May 22, 2017 13:14

added the close method to the base cache

97bd900

added close methods

8f322ba

close cache instance in the decorator

fd46217

Issue #233: Fixed resource clean-up in cached decorator.

added mock close method

1d7867e

close multicache decorator

7477e74

closed cache's in tests and examples.

cb874a3

missed one

952d7fe

fixed syntax

755c201

last one

6c65f81

argaen requested changes May 25, 2017

View reviewed changes

Quinny added 6 commits May 25, 2017 10:55

made changes as per @argaen's code review

e1aac65

increasing coverage

eff401f

test coverage

1ea684d

close does not require a connection

0afd3c7

must set within the test to force a connection

9a8fe6b

added test for when pool is None and close is called

41c4bb0

updated singleton example and removed dependacy on closing the connec…

c034795

…tion in decorators

argaen approved these changes May 25, 2017

View reviewed changes

argaen merged commit 333119e into aio-libs:master May 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#233 Added interface for performing resource clean up and updated tests #236

#233 Added interface for performing resource clean up and updated tests #236

Quinny commented May 23, 2017

codecov-io commented May 23, 2017 •

edited

Loading

argaen left a comment

argaen May 25, 2017

Quinny May 25, 2017 •

edited

Loading

argaen May 25, 2017

Quinny May 25, 2017

argaen May 25, 2017

Quinny May 25, 2017 •

edited

Loading

argaen May 25, 2017

Quinny May 25, 2017 •

edited

Loading

argaen May 25, 2017

Quinny May 25, 2017

argaen May 25, 2017

argaen May 25, 2017

argaen May 25, 2017

Quinny May 25, 2017

argaen May 25, 2017

Quinny May 25, 2017

Quinny commented May 25, 2017

argaen commented May 25, 2017

#233 Added interface for performing resource clean up and updated tests #236

#233 Added interface for performing resource clean up and updated tests #236

Conversation

Quinny commented May 23, 2017

codecov-io commented May 23, 2017 • edited Loading

Codecov Report

argaen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quinny May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quinny May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quinny May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quinny commented May 25, 2017

argaen commented May 25, 2017

codecov-io commented May 23, 2017 •

edited

Loading

Quinny May 25, 2017 •

edited

Loading

Quinny May 25, 2017 •

edited

Loading

Quinny May 25, 2017 •

edited

Loading