From 1e2755be30c7d1d0c5f757adf4fafa65b880e353 Mon Sep 17 00:00:00 2001 From: Alexey Igrychev Date: Thu, 23 Sep 2021 00:00:50 +0100 Subject: [PATCH] fix: sharing not thread safe go-git tree and storer --- pkg/build/stage/git_mapping.go | 4 +- pkg/git_repo/repo_handle/handle.go | 47 +++++++++++++++-- pkg/git_repo/repo_handle/interface.go | 28 +++++++---- pkg/git_repo/repo_handle/tree_handle.go | 49 ++++++++++++++++++ pkg/true_git/ls_tree/ls_tree.go | 67 ++++++------------------- pkg/true_git/ls_tree/result.go | 28 +---------- 6 files changed, 127 insertions(+), 96 deletions(-) create mode 100644 pkg/git_repo/repo_handle/tree_handle.go diff --git a/pkg/build/stage/git_mapping.go b/pkg/build/stage/git_mapping.go index 54ba550e5d..30810d07b0 100644 --- a/pkg/build/stage/git_mapping.go +++ b/pkg/build/stage/git_mapping.go @@ -926,12 +926,12 @@ func (gm *GitMapping) getArchiveType(ctx context.Context, commit string) (git_re func (gm *GitMapping) isEmpty(ctx context.Context, c Conveyor) (bool, error) { commitInfo, err := gm.GetLatestCommitInfo(ctx, c) if err != nil { - return true, err + return true, fmt.Errorf("unable to get latest commit info: %s", err) } pathScope, err := gm.getPathScope(ctx, commitInfo.Commit) if err != nil { - return true, err + return true, fmt.Errorf("unable to get path scope: %s", err) } isAnyMatchesByGitAdd, err := gm.GitRepo().IsAnyCommitTreeEntriesMatched(ctx, commitInfo.Commit, pathScope, gm.getPathMatcher(), true) diff --git a/pkg/git_repo/repo_handle/handle.go b/pkg/git_repo/repo_handle/handle.go index a66948dc51..3d4ebf7c01 100644 --- a/pkg/git_repo/repo_handle/handle.go +++ b/pkg/git_repo/repo_handle/handle.go @@ -2,30 +2,67 @@ package repo_handle import ( "fmt" + "io/ioutil" "sync" "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/config" + "github.com/go-git/go-git/v5/plumbing" ) type handle struct { repository *git.Repository submoduleHandleList []SubmoduleHandle - mutex sync.Mutex + mutex *sync.Mutex } -func newHandle(repository *git.Repository) *handle { +func newHandle(repository *git.Repository, mutex *sync.Mutex) *handle { return &handle{ repository: repository, + mutex: mutex, } } -func (h *handle) WithRepository(f func(r Repository) error) error { +func (h *handle) ReadBlobObjectContent(hash plumbing.Hash) ([]byte, error) { h.mutex.Lock() defer h.mutex.Unlock() - return f(h.repository) + obj, err := h.repository.BlobObject(hash) + if err != nil { + return nil, fmt.Errorf("unable to get blob %q object: %s", hash, err) + } + + f, err := obj.Reader() + if err != nil { + return nil, err + } + defer f.Close() + + data, err := ioutil.ReadAll(f) + if err != nil { + return nil, fmt.Errorf("unable to read blob %q content: %s", hash, err) + } + + return data, nil +} + +func (h *handle) GetCommitTree(hash plumbing.Hash) (TreeHandle, error) { + h.mutex.Lock() + defer h.mutex.Unlock() + + commit, err := h.repository.CommitObject(hash) + if err != nil { + return nil, fmt.Errorf("unable to get commit %q object: %s", hash, err) + } + + tree, err := commit.Tree() + if err != nil { + return nil, fmt.Errorf("unable to get commit %q tree: %s", hash, err) + } + + treeHandle := newTreeHandle(tree, h.mutex) + return treeHandle, nil } func (h *handle) Submodule(submodulePath string) (SubmoduleHandle, error) { @@ -48,7 +85,7 @@ type submoduleHandle struct { status *git.SubmoduleStatus } -func newRepositorySubmoduleHandle(handle Handle, config *config.Submodule, status *git.SubmoduleStatus) *submoduleHandle { +func newSubmoduleHandle(handle Handle, config *config.Submodule, status *git.SubmoduleStatus) *submoduleHandle { return &submoduleHandle{ Handle: handle, config: config, diff --git a/pkg/git_repo/repo_handle/interface.go b/pkg/git_repo/repo_handle/interface.go index 19e5e46183..3fe5971500 100644 --- a/pkg/git_repo/repo_handle/interface.go +++ b/pkg/git_repo/repo_handle/interface.go @@ -2,6 +2,7 @@ package repo_handle import ( "fmt" + "sync" "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/config" @@ -13,9 +14,17 @@ import ( // caching the necessary data from the worktree during initialization, // and then working exclusively with git objects. type Handle interface { - WithRepository(func(r Repository) error) error Submodule(submodulePath string) (SubmoduleHandle, error) Submodules() []SubmoduleHandle + ReadBlobObjectContent(hash plumbing.Hash) ([]byte, error) + GetCommitTree(hash plumbing.Hash) (TreeHandle, error) +} + +type TreeHandle interface { + Hash() plumbing.Hash + Entries() []object.TreeEntry + Tree(path string) (TreeHandle, error) + FindEntry(path string) (*object.TreeEntry, error) } type SubmoduleHandle interface { @@ -24,15 +33,14 @@ type SubmoduleHandle interface { Status() *git.SubmoduleStatus } -type Repository interface { - CommitObject(h plumbing.Hash) (*object.Commit, error) - BlobObject(h plumbing.Hash) (*object.Blob, error) +func NewHandle(repository *git.Repository) (Handle, error) { + return newHandleWithSubmodules(repository, &sync.Mutex{}) } -func NewHandle(repository *git.Repository) (Handle, error) { - h := newHandle(repository) +func newHandleWithSubmodules(repository *git.Repository, mutex *sync.Mutex) (Handle, error) { + h := newHandle(repository, mutex) - submoduleHandleList, err := getSubmoduleHandleList(repository) + submoduleHandleList, err := getSubmoduleHandleList(repository, mutex) if err != nil { return nil, err } @@ -42,7 +50,7 @@ func NewHandle(repository *git.Repository) (Handle, error) { return h, nil } -func getSubmoduleHandleList(parentRepository *git.Repository) ([]SubmoduleHandle, error) { +func getSubmoduleHandleList(parentRepository *git.Repository, mutex *sync.Mutex) ([]SubmoduleHandle, error) { var list []SubmoduleHandle w, err := parentRepository.Worktree() @@ -66,12 +74,12 @@ func getSubmoduleHandleList(parentRepository *git.Repository) ([]SubmoduleHandle return nil, fmt.Errorf("unable to get submodule %q status: %s", s.Config().Path, err) } - handle, err := NewHandle(submoduleRepository) + handle, err := newHandleWithSubmodules(submoduleRepository, mutex) if err != nil { return nil, err } - submoduleHandle := newRepositorySubmoduleHandle(handle, s.Config(), submoduleStatus) + submoduleHandle := newSubmoduleHandle(handle, s.Config(), submoduleStatus) list = append(list, submoduleHandle) } diff --git a/pkg/git_repo/repo_handle/tree_handle.go b/pkg/git_repo/repo_handle/tree_handle.go new file mode 100644 index 0000000000..631139faad --- /dev/null +++ b/pkg/git_repo/repo_handle/tree_handle.go @@ -0,0 +1,49 @@ +package repo_handle + +import ( + "fmt" + "sync" + + "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/object" +) + +type treeHandle struct { + tree *object.Tree + mutex *sync.Mutex +} + +func newTreeHandle(tree *object.Tree, mutex *sync.Mutex) *treeHandle { + return &treeHandle{tree: tree, mutex: mutex} +} + +func (h *treeHandle) Hash() plumbing.Hash { + return h.tree.Hash +} + +func (h *treeHandle) Tree(path string) (TreeHandle, error) { + h.mutex.Lock() + defer h.mutex.Unlock() + + tree, err := h.tree.Tree(path) + if err != nil { + return nil, fmt.Errorf("unable to get tree %q: %s", path, err) + } + + treeHandle := newTreeHandle(tree, h.mutex) + return treeHandle, nil +} + +func (h *treeHandle) FindEntry(path string) (*object.TreeEntry, error) { + h.mutex.Lock() + defer h.mutex.Unlock() + + return h.tree.FindEntry(path) +} + +func (h *treeHandle) Entries() []object.TreeEntry { + h.mutex.Lock() + defer h.mutex.Unlock() + + return h.tree.Entries +} diff --git a/pkg/true_git/ls_tree/ls_tree.go b/pkg/true_git/ls_tree/ls_tree.go index 223ccf7a10..b76b3662c5 100644 --- a/pkg/true_git/ls_tree/ls_tree.go +++ b/pkg/true_git/ls_tree/ls_tree.go @@ -80,7 +80,7 @@ func LsTree(ctx context.Context, repoHandle repo_handle.Handle, commit string, o func lsTree(ctx context.Context, repoHandle repo_handle.Handle, commit string, opts LsTreeOptions) (*Result, error) { res := NewResult(commit, "", []*LsTreeEntry{}, []*SubmoduleResult{}) - tree, err := getCommitTree(repoHandle, commit) + tree, err := repoHandle.GetCommitTree(plumbing.NewHash(commit)) if err != nil { return nil, err } @@ -112,7 +112,7 @@ func lsTree(ctx context.Context, repoHandle repo_handle.Handle, commit string, o TreeEntry: object.TreeEntry{ Name: "", Mode: filemode.Dir, - Hash: tree.Hash, + Hash: tree.Hash(), }, } @@ -151,33 +151,7 @@ func lsTree(ctx context.Context, repoHandle repo_handle.Handle, commit string, o return res, nil } -func getCommitTree(repoHandle repo_handle.Handle, commit string) (*object.Tree, error) { - commitHash, err := newHash(commit) - if err != nil { - return nil, fmt.Errorf("invalid commit %q: %s", commit, err) - } - - var tree *object.Tree - if err := repoHandle.WithRepository(func(repository repo_handle.Repository) error { - commitObj, err := repository.CommitObject(commitHash) - if err != nil { - return fmt.Errorf("unable to get %s commit info: %s", commit, err) - } - - tree, err = commitObj.Tree() - if err != nil { - return err - } - - return nil - }); err != nil { - return nil, err - } - - return tree, nil -} - -func processSpecificEntryFilepath(ctx context.Context, repoHandle repo_handle.Handle, tree *object.Tree, repositoryFullFilepath, treeFullFilepath, treeEntryFilepath string, opts LsTreeOptions) (lsTreeEntries []*LsTreeEntry, submodulesResults []*SubmoduleResult, err error) { +func processSpecificEntryFilepath(ctx context.Context, repoHandle repo_handle.Handle, tree repo_handle.TreeHandle, repositoryFullFilepath, treeFullFilepath, treeEntryFilepath string, opts LsTreeOptions) (lsTreeEntries []*LsTreeEntry, submodulesResults []*SubmoduleResult, err error) { for _, submoduleHandle := range repoHandle.Submodules() { submoduleEntryFilepath := filepath.FromSlash(submoduleHandle.Config().Path) submoduleFullFilepath := filepath.Join(treeFullFilepath, submoduleEntryFilepath) @@ -227,8 +201,8 @@ func processSpecificEntryFilepath(ctx context.Context, repoHandle repo_handle.Ha return lsTreeEntries, submodulesLsTreeEntries, nil } -func lsTreeWalk(ctx context.Context, repoHandle repo_handle.Handle, tree *object.Tree, repositoryFullFilepath, treeFullFilepath string, opts LsTreeOptions) (lsTreeEntries []*LsTreeEntry, submodulesResults []*SubmoduleResult, err error) { - for _, treeEntry := range tree.Entries { +func lsTreeWalk(ctx context.Context, repoHandle repo_handle.Handle, tree repo_handle.TreeHandle, repositoryFullFilepath, treeFullFilepath string, opts LsTreeOptions) (lsTreeEntries []*LsTreeEntry, submodulesResults []*SubmoduleResult, err error) { + for _, treeEntry := range tree.Entries() { lsTreeEntry := &LsTreeEntry{ FullFilepath: filepath.Join(treeFullFilepath, treeEntry.Name), TreeEntry: treeEntry, @@ -246,7 +220,7 @@ func lsTreeWalk(ctx context.Context, repoHandle repo_handle.Handle, tree *object return } -func lsTreeEntryMatch(ctx context.Context, repoHandle repo_handle.Handle, tree *object.Tree, repositoryFullFilepath, treeFullFilepath string, lsTreeEntry *LsTreeEntry, opts LsTreeOptions) (lsTreeEntries []*LsTreeEntry, submodulesResults []*SubmoduleResult, err error) { +func lsTreeEntryMatch(ctx context.Context, repoHandle repo_handle.Handle, tree repo_handle.TreeHandle, repositoryFullFilepath, treeFullFilepath string, lsTreeEntry *LsTreeEntry, opts LsTreeOptions) (lsTreeEntries []*LsTreeEntry, submodulesResults []*SubmoduleResult, err error) { switch lsTreeEntry.Mode { case filemode.Dir: return lsTreeDirEntryMatch(ctx, repoHandle, tree, repositoryFullFilepath, treeFullFilepath, lsTreeEntry, opts) @@ -257,7 +231,7 @@ func lsTreeEntryMatch(ctx context.Context, repoHandle repo_handle.Handle, tree * } } -func lsTreeDirEntryMatch(ctx context.Context, repoHandle repo_handle.Handle, tree *object.Tree, repositoryFullFilepath, treeFullFilepath string, lsTreeEntry *LsTreeEntry, opts LsTreeOptions) (lsTreeEntries []*LsTreeEntry, submodulesResults []*SubmoduleResult, err error) { +func lsTreeDirEntryMatch(ctx context.Context, repoHandle repo_handle.Handle, tree repo_handle.TreeHandle, repositoryFullFilepath, treeFullFilepath string, lsTreeEntry *LsTreeEntry, opts LsTreeOptions) (lsTreeEntries []*LsTreeEntry, submodulesResults []*SubmoduleResult, err error) { if err := lsTreeDirOrSubmoduleEntryMatchBase( lsTreeEntry.FullFilepath, opts, @@ -397,7 +371,7 @@ func lsTreeFileEntryMatch(ctx context.Context, lsTreeEntry *LsTreeEntry, opts Ls return } -func treeFindEntry(_ context.Context, tree *object.Tree, treeFullFilepath, treeEntryFilepath string) (*LsTreeEntry, error) { +func treeFindEntry(_ context.Context, tree repo_handle.TreeHandle, treeFullFilepath, treeEntryFilepath string) (*LsTreeEntry, error) { formattedTreeEntryPath := filepath.ToSlash(treeEntryFilepath) treeEntry, err := tree.FindEntry(formattedTreeEntryPath) if err != nil { @@ -410,7 +384,7 @@ func treeFindEntry(_ context.Context, tree *object.Tree, treeFullFilepath, treeE }, nil } -func treeTree(tree *object.Tree, treeFullFilepath, treeDirEntryFullFilepath string) (*object.Tree, error) { +func treeTree(tree repo_handle.TreeHandle, treeFullFilepath, treeDirEntryFullFilepath string) (repo_handle.TreeHandle, error) { treeDirEntryFilepath, err := filepath.Rel(treeFullFilepath, treeDirEntryFullFilepath) if err != nil || treeDirEntryFilepath == "." || treeDirEntryFilepath == ".." || strings.HasPrefix(treeDirEntryFilepath, ".."+string(os.PathSeparator)) { panic(fmt.Sprintf("unexpected paths: %s, %s", treeFullFilepath, treeDirEntryFullFilepath)) @@ -425,28 +399,15 @@ func treeTree(tree *object.Tree, treeFullFilepath, treeDirEntryFullFilepath stri return entryTree, nil } -func getSubmoduleTree(repoHandle repo_handle.SubmoduleHandle) (*object.Tree, error) { +func getSubmoduleTree(repoHandle repo_handle.SubmoduleHandle) (repo_handle.TreeHandle, error) { submoduleName := repoHandle.Config().Name expectedCommit := repoHandle.Status().Expected - - var submoduleTree *object.Tree - if err := repoHandle.WithRepository(func(submoduleRepository repo_handle.Repository) error { - commit, err := submoduleRepository.CommitObject(expectedCommit) - if err != nil { - return fmt.Errorf("cannot inspect submodule %q commit %q: %s", submoduleName, expectedCommit, err) - } - - submoduleTree, err = commit.Tree() - if err != nil { - return fmt.Errorf("cannot get submodule %q commit %q tree: %s", submoduleName, expectedCommit, err) - } - - return nil - }); err != nil { - return nil, err + tree, err := repoHandle.GetCommitTree(expectedCommit) + if err != nil { + return nil, fmt.Errorf("unable to get submodule %q commit %q tree: %s", submoduleName, expectedCommit, err) } - return submoduleTree, nil + return tree, nil } func debug() bool { diff --git a/pkg/true_git/ls_tree/result.go b/pkg/true_git/ls_tree/result.go index 3b29e462f8..be49aa732d 100644 --- a/pkg/true_git/ls_tree/result.go +++ b/pkg/true_git/ls_tree/result.go @@ -4,7 +4,6 @@ import ( "context" "crypto/sha256" "fmt" - "io/ioutil" "path/filepath" "sort" @@ -103,7 +102,7 @@ func (r *Result) LsTree(ctx context.Context, repoHandle repo_handle.Handle, opts func (r *Result) lsTree(ctx context.Context, repoHandle repo_handle.Handle, opts LsTreeOptions) (*Result, error) { res := NewResult(r.commit, r.repositoryFullFilepath, []*LsTreeEntry{}, []*SubmoduleResult{}) - tree, err := getCommitTree(repoHandle, r.commit) + tree, err := repoHandle.GetCommitTree(plumbing.NewHash(r.commit)) if err != nil { return nil, err } @@ -330,28 +329,5 @@ func (r *Result) LsTreeEntryContent(mainRepoHandle repo_handle.Handle, relPath s panic("unexpected condition") } - var data []byte - if err := entryRepoHandle.WithRepository(func(entryRepository repo_handle.Repository) error { - obj, err := entryRepository.BlobObject(entry.Hash) - if err != nil { - return fmt.Errorf("unable to get tree entry %q blob object: %s", entry.FullFilepath, err) - } - - f, err := obj.Reader() - if err != nil { - return err - } - defer f.Close() - - data, err = ioutil.ReadAll(f) - if err != nil { - return fmt.Errorf("unable to read tree entry %q content: %s", relPath, err) - } - - return nil - }); err != nil { - return nil, err - } - - return data, nil + return entryRepoHandle.ReadBlobObjectContent(entry.Hash) }