diff options
author | Peter Gardfjäll <peter.gardfjall.work@gmail.com> | 2022-08-29 11:45:20 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-08-29 11:45:20 +0200 |
commit | 4562d40fcead66e54525f710875377ebf7c4766e (patch) | |
tree | 6d77cf4742ce0dd6111a66fd5e4740a0954cce97 /services | |
parent | 41c76ad71404bc0b4da83df063e6e10cd4a41e9f (diff) | |
download | gitea-4562d40fcead66e54525f710875377ebf7c4766e.tar.gz gitea-4562d40fcead66e54525f710875377ebf7c4766e.zip |
fix hard-coded timeout and error panic in API archive download endpoint (#20925)
* fix hard-coded timeout and error panic in API archive download endpoint
This commit updates the `GET /api/v1/repos/{owner}/{repo}/archive/{archive}`
endpoint which prior to this PR had a couple of issues.
1. The endpoint had a hard-coded 20s timeout for the archiver to complete after
which a 500 (Internal Server Error) was returned to client. For a scripted
API client there was no clear way of telling that the operation timed out and
that it should retry.
2. Whenever the timeout _did occur_, the code used to panic. This was caused by
the API endpoint "delegating" to the same call path as the web, which uses a
slightly different way of reporting errors (HTML rather than JSON for
example).
More specifically, `api/v1/repo/file.go#GetArchive` just called through to
`web/repo/repo.go#Download`, which expects the `Context` to have a `Render`
field set, but which is `nil` for API calls. Hence, a `nil` pointer error.
The code addresses (1) by dropping the hard-coded timeout. Instead, any
timeout/cancelation on the incoming `Context` is used.
The code addresses (2) by updating the API endpoint to use a separate call path
for the API-triggered archive download. This avoids producing HTML-errors on
errors (it now produces JSON errors).
Signed-off-by: Peter Gardfjäll <peter.gardfjall.work@gmail.com>
Diffstat (limited to 'services')
-rw-r--r-- | services/repository/archiver/archiver.go | 71 |
1 files changed, 61 insertions, 10 deletions
diff --git a/services/repository/archiver/archiver.go b/services/repository/archiver/archiver.go index ebd3eaf236..ae43503bae 100644 --- a/services/repository/archiver/archiver.go +++ b/services/repository/archiver/archiver.go @@ -57,6 +57,21 @@ func (ErrUnknownArchiveFormat) Is(err error) bool { return ok } +// RepoRefNotFoundError is returned when a requested reference (commit, tag) was not found. +type RepoRefNotFoundError struct { + RefName string +} + +// Error implements error. +func (e RepoRefNotFoundError) Error() string { + return fmt.Sprintf("unrecognized repository reference: %s", e.RefName) +} + +func (e RepoRefNotFoundError) Is(err error) bool { + _, ok := err.(RepoRefNotFoundError) + return ok +} + // NewRequest creates an archival request, based on the URI. The // resulting ArchiveRequest is suitable for being passed to ArchiveRepository() // if it's determined that the request still needs to be satisfied. @@ -103,7 +118,7 @@ func NewRequest(repoID int64, repo *git.Repository, uri string) (*ArchiveRequest } } } else { - return nil, fmt.Errorf("Unknow ref %s type", r.refName) + return nil, RepoRefNotFoundError{RefName: r.refName} } return r, nil @@ -115,6 +130,49 @@ func (aReq *ArchiveRequest) GetArchiveName() string { return strings.ReplaceAll(aReq.refName, "/", "-") + "." + aReq.Type.String() } +// Await awaits the completion of an ArchiveRequest. If the archive has +// already been prepared the method returns immediately. Otherwise an archiver +// process will be started and its completion awaited. On success the returned +// RepoArchiver may be used to download the archive. Note that even if the +// context is cancelled/times out a started archiver will still continue to run +// in the background. +func (aReq *ArchiveRequest) Await(ctx context.Context) (*repo_model.RepoArchiver, error) { + archiver, err := repo_model.GetRepoArchiver(ctx, aReq.RepoID, aReq.Type, aReq.CommitID) + if err != nil { + return nil, fmt.Errorf("models.GetRepoArchiver: %v", err) + } + + if archiver != nil && archiver.Status == repo_model.ArchiverReady { + // Archive already generated, we're done. + return archiver, nil + } + + if err := StartArchive(aReq); err != nil { + return nil, fmt.Errorf("archiver.StartArchive: %v", err) + } + + poll := time.NewTicker(time.Second * 1) + defer poll.Stop() + + for { + select { + case <-graceful.GetManager().HammerContext().Done(): + // System stopped. + return nil, graceful.GetManager().HammerContext().Err() + case <-ctx.Done(): + return nil, ctx.Err() + case <-poll.C: + archiver, err = repo_model.GetRepoArchiver(ctx, aReq.RepoID, aReq.Type, aReq.CommitID) + if err != nil { + return nil, fmt.Errorf("repo_model.GetRepoArchiver: %v", err) + } + if archiver != nil && archiver.Status == repo_model.ArchiverReady { + return archiver, nil + } + } + } +} + func doArchive(r *ArchiveRequest) (*repo_model.RepoArchiver, error) { txCtx, committer, err := db.TxContext() if err != nil { @@ -147,11 +205,7 @@ func doArchive(r *ArchiveRequest) (*repo_model.RepoArchiver, error) { } } - rPath, err := archiver.RelativePath() - if err != nil { - return nil, err - } - + rPath := archiver.RelativePath() _, err = storage.RepoArchives.Stat(rPath) if err == nil { if archiver.Status == repo_model.ArchiverGenerating { @@ -284,13 +338,10 @@ func StartArchive(request *ArchiveRequest) error { } func deleteOldRepoArchiver(ctx context.Context, archiver *repo_model.RepoArchiver) error { - p, err := archiver.RelativePath() - if err != nil { - return err - } if err := repo_model.DeleteRepoArchiver(ctx, archiver); err != nil { return err } + p := archiver.RelativePath() if err := storage.RepoArchives.Delete(p); err != nil { log.Error("delete repo archive file failed: %v", err) } |