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

Python destructor refactoring and exception safety #807

Merged
merged 3 commits into from Sep 20, 2016

Conversation

Projects
None yet
6 participants
@trws
Copy link
Member

trws commented Sep 13, 2016

This is an early pull request that still needs some cleanup, but adds uniform destruction support to all wrapped objects, and as part of that reorganizes the init calls and so-forth to better handle exceptions. Chances are this will help with the resource usage problems.

@SteVwonder

This comment has been minimized.

Copy link
Member

SteVwonder commented Sep 14, 2016

Looks good to me! The deconstructor argument for Wrapper is a nice touch.

@@ -5,6 +5,9 @@
import cffi
from types import MethodType

import traceback

This comment has been minimized.

@SteVwonder

SteVwonder Sep 14, 2016

Member

unused import?

This comment has been minimized.

@trws

trws Sep 14, 2016

Author Member

Thanks for the catch, I had that in there to print stack traces while debugging a free before use issue. Will post an update later today.

On September 14, 2016 at 7:04:27 AM PDT, Stephen Herbein notifications@github.com wrote:

In src/bindings/python/flux/wrapper.pyhttps://github.com//pull/807#discussion_r78754406:

@@ -5,6 +5,9 @@
import cffi
from types import MethodType

+import traceback

unused import?

You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHubhttps://github.com//pull/807/files/a3ec52d781e81d9b994dc209383c11117c8bac05#r78754406, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAoStXEakYDxD32pDWO7DU1XZhB3q_mmks5qp_7igaJpZM4J8O6W.

@trws trws force-pushed the trws:rpc-cleanup branch from 1d1abbe to 56a63e5 Sep 14, 2016

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 14, 2016

Ok, more cleanup, and should handle resource destruction much better than the recent versions have. The other thing is that all wrapped objects are now usable as context objects. I'm still waffling on this a bit, as it means KVSDir objects commit at the end of a block while everything else is destructed at the end of such a block, but it seems like a good way to handle many common patterns without having to worry about reference cycle issues in the garbage collector.

What do you think @SteVwonder?

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 14, 2016

Current coverage is 74.92% (diff: 100%)

Merging #807 into master will decrease coverage by <.01%

@@             master       #807   diff @@
==========================================
  Files           145        145          
  Lines         24911      24911          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          18666      18665     -1   
- Misses         6245       6246     +1   
  Partials          0          0          

Powered by Codecov. Last update b532e12...37cbb65

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage increased (+0.08%) to 75.314% when pulling 56a63e5 on trws:rpc-cleanup into 430bedb on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage increased (+0.02%) to 75.256% when pulling 682210f on trws:rpc-cleanup into 430bedb on flux-framework:master.

@SteVwonder

This comment has been minimized.

Copy link
Member

SteVwonder commented Sep 14, 2016

@trws: The addition of the context methods to the Wrapper class is a slick one. I like it a lot. Just to confirm some assumptions:

  1. Wrapped objects that aren't used as context objects will be eligible for GC (and thus proper deconstruction) when the variable holding them goes out of scope, correct? AKA, you don't have to use the context to get proper deconstruction, correct? I assume this is the case since __del__ calls __clear.
  2. If you use KVSDirs as a context object, you can still commit before exiting the block, correct? If so, I think this is fine. Having the commit in the __exit__ makes sense, but isn't a limitation since users can still commit additionally within the block if necessary. Clarifying example:
with flux.kvs.KVSDir(handle, "some_dir_key") as some_dir:
    some_dir['new_key'] = 'foo'
    some_dir.commit()
    some_dir['new_key_2'] = 'bar'

I assume that in the above example both keys will be created and that two separate calls will be made to kvs_commit (once due to the user's explicit code and once due to exiting the KVSDir context block).
My only minor nit with calling commit in __exit__ is that it could incur an additional, unnecessary commit. Does KVS library optimize for this case? Or does it cause an empty commit to occur?

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 14, 2016

  1. Correct, with the clarification that the only guarantee is that they will be cleaned up eventually. In CPython it would be when the last reference to it is destroyed, in PyPy or some other like that it might be at any time after the last reference goes away. In other words, yes they're eligible for normal collection.
  2. Yup, you can call commit as many times as you like. I had that as a context manager from the start just because it seemed pretty common to want to get a directory, make some modifications, then commit them all regardless of how one exited the region. It makes that a bit easier, since it didn't make sense to have the commit in the destructor (since you might just want to read the directory rather than write to it). It might incur unnecessary overhead as you say, but the idea was to make it so that it would only incur that overhead if the user explicitly uses it as a context then doesn't do any writes.

The part I'm not sure I like is that the KVSDir context behaves differently from the others. Maybe it isn't confusing, but it seems like it could be.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 14, 2016

I may not be understanding the python use case here, and whatever you two decide is fine with me, but one concern I would have is unecessary or undesired kvs_commit happening from the use of KVSDir objects used only for reading. Is that a valid case?

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 14, 2016

If a user writes:

with kvs.get_dir(fh, "path") as d:
  read_stuff_but_dont_write_anything(d)

Then they'll get a commit they don't need, but only because they put it in a with block. If it were written this way:

d = kvs.get_dir(fh, "path")
read_stuff_but_dont_write_anything(d)
<d eventually destructed>

then there is no automatic commit. The main use-case for a context manager like that is for cases where you want to make sure a KVSDir gets committed even if you run into an exception or early return inside the block. Alternately I could remove the commit context from the KVSDir object and make it a separate thing, usable something like this:

with kvs.committer(kvs.get_dir(fh, "path")) as d:
  do_stuff_that_may_write_and_may_except(d)

Would that be more palatable?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 14, 2016

No, what you have is fine. My problem was I didn't understand python context blocks.

Sorry, ignore the weirdo in the corner, please!

@SteVwonder

This comment has been minimized.

Copy link
Member

SteVwonder commented Sep 15, 2016

@trws: I vote for keeping the commit in the context block. It seems to be the cleanest way to do it, and removing would probably trip more people up than the other way around.

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 20, 2016

Any further adjustments to make this ready? @SteVwonder, @grondo?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 20, 2016

I don't have any comments on code changes, but the fixup commit for the unused import could be squashed into the commit in which the import was added to make that disappear from history.

Also, the first commit subject should be something like python: add explicit rpc cleanup

@SteVwonder
Copy link
Member

SteVwonder left a comment

Overall looks good.

Sidenote: We should probably start running pylint or some other linter on PRs, but I don't think that should block this one.

@@ -4,6 +4,7 @@
import json
import collections
import errno
import sys

This comment has been minimized.

@SteVwonder

SteVwonder Sep 20, 2016

Member

Unused import?

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 20, 2016

That's a really good point @SteVwonder, I've been using flake8 for some
other projects, and it's a standard part of make check on spack now.
I'll add that to another PR with a separate commit to make it actually
pass so we can isolate that, it'll probably be significant.

On 20 Sep 2016, at 8:40, Stephen Herbein wrote:

SteVwonder approved this pull request.

Overall looks good.

Sidenote: We should probably start running pylint or some other linter
on PRs, but I don't think that should block this one.

@@ -4,6 +4,7 @@
import json
import collections
import errno
+import sys

Unused import?

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#807 (review)

trws added some commits Sep 13, 2016

python: explicit rpc destruction
This may have been leaking RPCs, it may also mean others have been
leaking handles.
python: destructor for sec structure
Ensure sec structure is cleaned up, and clear out an unused import.

@trws trws force-pushed the trws:rpc-cleanup branch from 682210f to 37cbb65 Sep 20, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 20, 2016

Coverage Status

Coverage decreased (-0.004%) to 75.245% when pulling 37cbb65 on trws:rpc-cleanup into b532e12 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 20, 2016

Sorry in advance for the OT github question, but @SteVwonder how did you "approve" the changes?

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 20, 2016

It's the new reviews feature, they posted about it on the blog sometime in the last week or so. Not a big change, but allows several comments and an approval or request for changes to be lumped into a single "review."

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 20, 2016

Look set to you @grondo?

I'm working on another PR to add pylint or flake8 checks BTW @SteVwonder, was planning on flake8, but it looks like we have a version of pylint available already (didn't think so, but we do) on LC so that may be the way to go.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 20, 2016

Great, thanks for humoring me with that minor cleanup!

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 20, 2016

Ok to merge @trws?

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 20, 2016

Yup, good to go, automatic syntax checking coming separately.

On 20 Sep 2016, at 13:13, Mark Grondona wrote:

Ok to merge @trws?

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#807 (comment)

@grondo grondo merged commit 914fb9f into flux-framework:master Sep 20, 2016

4 checks passed

codecov/patch Coverage not affected when comparing b532e12...37cbb65
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +25.06% compared to b532e12
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.004%) to 75.245%
Details

@grondo grondo removed the review label Sep 20, 2016

@garlick garlick referenced this pull request Oct 26, 2016

Closed

0.5.0 release notes #879

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.