Skip to content

fix hard-coded timeout and error panic in API archive download endpoint#20925

Merged
6543 merged 2 commits into
go-gitea:mainfrom
petergardfjall:fix-archive-download-error
Aug 29, 2022
Merged

fix hard-coded timeout and error panic in API archive download endpoint#20925
6543 merged 2 commits into
go-gitea:mainfrom
petergardfjall:fix-archive-download-error

Conversation

@petergardfjall

@petergardfjall petergardfjall commented Aug 23, 2022

Copy link
Copy Markdown
Contributor

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).

@petergardfjall petergardfjall force-pushed the fix-archive-download-error branch from f606db1 to e3e563a Compare August 23, 2022 12:06
petergardfjall
petergardfjall commented Aug 23, 2022
View reviewed changes
Comment thread routers/api/v1/repo/file.go Outdated

@petergardfjall petergardfjall Aug 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this is very similar to web/repo/repo.go#Download (only difference is the type of context in the function signature). That is unfortunate but I didn't see any obvious way of using the same code for these since they produce very different error responses (json vs html).

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 23, 2022
petergardfjall
petergardfjall commented Aug 23, 2022
View reviewed changes
Comment thread cmd/migrate_storage.go Outdated
@petergardfjall petergardfjall force-pushed the fix-archive-download-error branch 3 times, most recently from 8c93a06 to 0e7dda6 Compare August 23, 2022 14:44
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 26, 2022
@lunny lunny mentioned this pull request Aug 26, 2022
Merged
@lunny lunny added type/enhancement An improvement of existing functionality backport/v1.17 skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Aug 26, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 26, 2022
@6543

6543 commented Aug 26, 2022

Copy link
Copy Markdown
Member

@petergardfjall latest ci issue is related ... please ajust to new code

@prologic

prologic commented Aug 27, 2022

Copy link
Copy Markdown

Does this PR hopefully solve some 5xx errors I'm seeing in my instance, like:

2022/08/25 15:04:16 [63078f70-7] router: completed HEAD /prologic/go-gopher/archive/:go_gopher/i0d1jMZx_shouldntexist.tar.gz for 96.47.72.93:0, 500 Internal Server Error in 8.8ms @ repo/repo.go:386(repo.Download)

@lunny

lunny commented Aug 28, 2022

Copy link
Copy Markdown
Member

:go_gopher

Maybe not, why there is a :go_gopher in the url.

@delvh

delvh commented Aug 28, 2022

Copy link
Copy Markdown
Member

Sounds to me like the recent PR we already merged with the archiver using the branch instead of the specified commit?

@petergardfjall

petergardfjall commented Aug 29, 2022

Copy link
Copy Markdown
Contributor Author

@petergardfjall latest ci issue is related ... please ajust to new code

CI is green now. ✔️

@petergardfjall
fix hard-coded timeout and error panic in API archive download endpoint
a611fb8 
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>
@petergardfjall
adapt to recent ServeStream name change
f129dbb
@petergardfjall petergardfjall force-pushed the fix-archive-download-error branch from 42129a9 to f129dbb Compare August 29, 2022 08:53
@6543

6543 commented Aug 29, 2022

Copy link
Copy Markdown
Member

.

@6543 6543 merged commit 4562d40 into go-gitea:main Aug 29, 2022
@petergardfjall petergardfjall deleted the fix-archive-download-error branch August 29, 2022 09:53
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 30, 2022
@zjjhot
Merge remote-tracking branch 'upstream/main'
b2861a8 
* upstream/main:
  Fix typo (go-gitea#20993)
  fix broken insecureskipverify handling in rediss connection uris (go-gitea#20967)
  Redirect if user does not exist (go-gitea#20981)
  fix hard-coded timeout and error panic in API archive download endpoint (go-gitea#20925)
  Add support for Vagrant packages (go-gitea#20930)
@6543

6543 commented Aug 30, 2022

Copy link
Copy Markdown
Member

please backport :)

zeripath pushed a commit to zeripath/gitea that referenced this pull request Sep 4, 2022
@petergardfjall @zeripath
fix hard-coded timeout and error panic in API archive download endpoi…
3dfb20d 
…nt (go-gitea#20925)

Backport go-gitea#20925

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>
@zeripath zeripath mentioned this pull request Sep 4, 2022
Merged
@zeripath zeripath added the backport/done All backports for this PR have been created label Sep 4, 2022
zeripath added a commit that referenced this pull request Sep 6, 2022
@zeripath @petergardfjall @lunny
Fix hard-coded timeout and error panic in API archive download endpoi…
06f968d 
…nt (#20925) (#21051)

Backport #20925

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>

Signed-off-by: Peter Gardfjäll <peter.gardfjall.work@gmail.com>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: Peter Gardfjäll <peter.gardfjall.work@gmail.com>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@wxiaoguang wxiaoguang added this to the 1.18.0 milestone Oct 7, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lunny lunny lunny approved these changes

@6543 6543 6543 approved these changes

Assignees

No one assigned

Labels

backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/enhancement An improvement of existing functionality

Projects

None yet

Milestone

1.18.0

Development

Successfully merging this pull request may close these issues.

8 participants

@petergardfjall @6543 @prologic @lunny @delvh @zeripath @wxiaoguang @GiteaBot