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

Eliminate Leaks by wrapping resources in contextlib with-blocks #36

Closed
wants to merge 21 commits into from

Conversation

ankostis
Copy link
Contributor

@ankostis ankostis commented Oct 24, 2016

  • BREAKING API: retrofit streams and packers as context-managers.
    Specifically packers (git.pack.PackIndexFile, git.pack.PackFile & git.pack.PackEntity)
    must always be used from within a with .. block, or you will get mmap-cursor missing errors.

    .. Note::

    You can "enter" `PackIndexFile&PackFile``multiple time, but``PackEntity``only once
    to detect and avoid sloppy deep-nesting.
    Since``git.pack.PackEntity``class just coalseces``PackIndexFile``&``PackFile``,
    you may "enter" either each internal packer separately, or the entity only once.

  • BREAKING API: some utilities moved between git.util, git.const & git.utils.compat.

  • Fix (probably) all leaks in Windows.

    .. Note::

    The problem is that on Linux, any open files go unoticed, or collected by GC.
    But on Windows (and specifically on PY3 where GC is not deterministic),
    the underlying files cannot delete due to access violation.

    That's a Good-thing©, because it is dangerous to leak memory-mapped handles.
    Actually Windows may leak them even after process who created them have died,
    needing a full restart(!) to clean them up (signing-out is not enough).

  • Stop depending on smmap submodule - deleted completely.
    Developer now has to specify any specific dependecy to smmap in requirements.txt file, and
    remember to updated before a final release.

+ fix(loose-db): fix bad-attr in ex-message
+ feat(util): add logger.
+ feat(util): add suppress-ex context-handler (from PY3 sources).
+ Packers MUST be invoked inside `Withh...` blocks, or `_cursor` won't
exist!
+ Had to drop NotLazy for their hierarchy :-(
+ Count entrances/exits.
+ feat(util: add `rmtree()` for READ_ONLY files on Windows.

3-->2  Windows TCs now fail.
+ stop importing git-submodules for gitdb & smmap
+ File-in-use errors were fixed with `gitdb.util.mman.collect()`!
+ This call is disabled `gitdb.util.HIDE_WINDOWS_KNOWN_ERRORS == False`.
+ Depend on latest smmp `v2.1.0.dev1` tag
Speed-up for non-error commits:
+  no-leak contextlibing(v.2.0.0, 941b6c7): 5.20
+  Prev commit, no LazyMixin(v.2.1.0.dev1): 7.70
+  This commit, LazyMixin: 5.50
+ chrore(deps): depend on *contextlib2* for `ExitStack` in PY2.
+ refact(util): BREAKING API move consts out of utils.
+ style(pep8): fixe all sources.
speedups:
+ 3.5: ~5% faster
+ 3.4: ~15% faster
+ 3.3/2.7/2.6: the same

BREAKING API 1: `XXOStream` not an instance of `XXOInfo`.
BREAKING API 2: `XXOinfo/XXOStream not inheritable.
(ie no `DeriveTest` class).
+ chore(deps): 
  + smmap2-v2.1.0.dev4 (FIXes memoryview leak).
  + FIX quoted environment marker in requirements.
  + import actually smmap2!
@Byron
Copy link
Member

Byron commented Dec 8, 2016

@ankostis Did you delete the branch because gitdb is only optional in GitPython? I am asking because I thought this one was close to being mergeable, given the immense amount of work that went into it.

@ankostis
Copy link
Contributor Author

ankostis commented Dec 8, 2016

Yes that was one of the reasons.

You see, this PR still gitdb didn't behave correctly, and then I discovered the memoryview leaks in smmap lib and moved into this project to fix it, and in the process, I had to do massive API-breaking changes. See this personal branch which is a total rewrite based on the conclusions I had layed out in gitpython-developers/smmap/issues/31#issuecomment-256004958.

Then I was planning to reimplement this PR based on the new smmap but, 1) i had other priorities, and 2) I knew that would require more work on the GitPython project to re-enact the use of GitDb, and I couldn't see when I would have the time for all this, so I kind-of abandoned it,

@Byron
Copy link
Member

Byron commented Dec 8, 2016

Alright, thanks for the summary :)! That's how it happens sometimes. And having one functional backend is probably sufficient anyway - pure-python (i.e. gitdb) is not really an advantage anyways, just because the cgit executable is available everywhere.

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

Successfully merging this pull request may close these issues.

None yet

2 participants