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

GenericRepository: fix unpickling of objects #455

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

OddBloke
Copy link
Contributor

This allows them to be passed across process boundaries when using concurrent.futures.ProcessPoolExecutor. Without this, a RecursionError occurs.

The issue stems from GenericRepository.__getattr__ calling getattr(self.path, ...). GenericRepository.path is a property, so it is found, but it then calls self._artifactory.joinpath(...) and self._artifactory is not yet present on the partially-restored GenericRepository, resulting in a call to
GenericRepository.__getattr__, etc.

This fix ensures that self._artifactory will be restored to the object early enough in unpickling to avoid the issue.

This allows them to be passed across process boundaries when using
`concurrent.futures.ProcessPoolExecutor`.  Without this, a
`RecursionError` occurs.

The issue stems from `GenericRepository.__getattr__` calling
`getattr(self.path, ...)`.  `GenericRepository.path` is a property, so
it is found, but it then calls `self._artifactory.joinpath(...)` and
`self._artifactory` is not yet present on the partially-restored
`GenericRepository`, resulting in a call to
`GenericRepository.__getattr__`, etc.

This fix ensures that `self._artifactory` will be restored to the object
early enough in unpickling to avoid the issue.
@allburov allburov merged commit e2d8ce7 into devopshq:master Sep 26, 2024
4 checks passed
@allburov
Copy link
Member

TY! 🫶

@OddBloke OddBloke deleted the oddbloke/pickle branch September 26, 2024 14:22
@OddBloke
Copy link
Contributor Author

@allburov Thank you for the speedy review and merge! Would it be possible to cut a new release including this fix?

@OddBloke
Copy link
Contributor Author

Actually, scratch that: it seems that we don't retain self.session.auth across process boundaries, so something more is needed here.

@OddBloke
Copy link
Contributor Author

I believe #456 will address that issue.

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.

2 participants