Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork6.3k
Limit reading bytes instead of ReadAll#35928
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
ChristopherHX commentedNov 11, 2025
I would possible tend to create a single file listing these limits as constant's, so finding them becomes slightly more structured. |
wxiaoguang commentedNov 11, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Although I also thought about the "constants", but I didn't find a proper package to contain them. And the more changes we made here, the more potential conflicts when backporting. At the moment, |
ChristopherHX commentedNov 12, 2025
I understand your point, will try this out tomorrow. I can currently not estimate how this constraints will become visible to me in the UI / api. |
wxiaoguang commentedNov 12, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There should be no "visible effect" in daily usage. When a file exceeds the limit, it will be only partially read (no error). In most cases, I used 1M (1024*1024), it should be large enough for a workflow file or a template file. I can't imagine how a normal file can exceed that limit. Update: BTW: this PR is not related to |
wxiaoguang commentedNov 12, 2025
Doesn't block 1.25.2 release, no need to wait for this. |
delvh left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's hope that 1MB is really large enough that no user ever encounters it.
If not, we can still adjust the limits.
| } | ||
| deferreader.Close() | ||
| content,err:=io.ReadAll(reader) | ||
| content,err:=util.ReadWithLimit(reader,5*1024*1024)// 5MB should be enough for a wiki page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually, I can imagine one case where a wiki page needs more:<img src="data:image/png;base64,…">
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure 🤷
So maybe 5M, 10M, 50M, 100M .....
But I guess there is a "general" limit that most browsers will stop responding when the content is large enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Chrome Mobile, doing it far more early than Chrome Desktop this limit UI responsive limit is artificial.
Loading the data works in most cases if below 4GB at least on desktop, but displaying not without extremely cautious paging out text from the html dom (scrolling is a problem then as well) e.g. place holder or help of SPA frameworks that do it for you.
Displaying CI logs in browsers without truncation is a challenge.
wxiaoguang commentedNov 12, 2025
IMO 1M is large enough in all cases. As large as QuickJS (about 59K lines):https://github.com/bellard/quickjs/blob/master/quickjs.c#L58923 , it is 1.85MB |
ChristopherHX left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would actually record an enhancement to report a failure for workflows exceeding the limit.
Currently a subset of the workflow might run and you have trouble to understand why jobs, steps or properties are missing and why sometimes the workflow just do not trigger because of a yaml syntax error.
| } | ||
| deferreader.Close() | ||
| content,err:=io.ReadAll(reader) | ||
| content,err:=util.ReadWithLimit(reader,5*1024*1024)// 5MB should be enough for a wiki page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Chrome Mobile, doing it far more early than Chrome Desktop this limit UI responsive limit is artificial.
Loading the data works in most cases if below 4GB at least on desktop, but displaying not without extremely cautious paging out text from the html dom (scrolling is a problem then as well) e.g. place holder or help of SPA frameworks that do it for you.
Displaying CI logs in browsers without truncation is a challenge.
wxiaoguang commentedNov 12, 2025
At the moment I can't imagine a workflow file exceeds 1MB ....
I think " Improve online runner check#35722 " started the initial supports for similar cases? |
372d24b intogo-gitea:mainUh oh!
There was an error while loading.Please reload this page.
* giteaofficial/main: Fix diff blob excerpt expansion (go-gitea#35922) Add GITEA_PR_INDEX env variable to githooks (go-gitea#35938) Fix container push tag overwriting (go-gitea#35936) Upgrade deps golang.org/x/crypto (go-gitea#35952) Fix corrupted external render content (go-gitea#35946) Don't show unnecessary error message to end users for DeleteBranchAfterMerge (go-gitea#35937) Limit reading bytes instead of ReadAll (go-gitea#35928)
* giteaofficial/release/v1.25: (77 commits) Add "site admin" back to profile menu (go-gitea#36010) (go-gitea#36013) release notes for 1.25.2 (go-gitea#35986) Allow empty commit when merging pull request with squash style (go-gitea#35989) (go-gitea#36003) Fix various permission & login related bugs (go-gitea#36002) (go-gitea#36004) upgrade golang.org/x/crypto to 0.45.0 (go-gitea#35988) Change project default column icon to 'star' (go-gitea#35967) (go-gitea#35979) Misc CSS fixes (go-gitea#35888) (go-gitea#35981) Fix container push tag overwriting (go-gitea#35936) (go-gitea#35954) Fix corrupted external render content (go-gitea#35946) (go-gitea#35950) Don't show unnecessary error message to end users for DeleteBranchAfterMerge (go-gitea#35937) (go-gitea#35941) Limit read bytes instead of ReadAll (go-gitea#35928) (go-gitea#35934) Load jQuery as early as possible to support custom scripts (go-gitea#35926) (go-gitea#35929) Allow to display embed images/pdfs when SERVE_DIRECT was enabled on MinIO storage (go-gitea#35882) (go-gitea#35917) Use correct form field for allowed force push users in branch protection API (go-gitea#35894) (go-gitea#35908) Make OAuth2 issuer configurable (go-gitea#35915) (go-gitea#35916)Fixgo-gitea#35763: Add proper page title for project pages (go-gitea#35773) (go-gitea#35909) Display source code downloads last for release attachments (go-gitea#35897) (go-gitea#35903) Fix team member access check (go-gitea#35899) (go-gitea#35905) Fix conda null depend issue (go-gitea#35900) (go-gitea#35902) Fix avatar upload error handling (go-gitea#35887) (go-gitea#35890) ...# Conflicts:#go.mod#go.sum#models/actions/run_test.go#models/fixtures/action_run.yml#models/fixtures/action_run_job.yml#models/fixtures/action_task.yml#models/fixtures/branch.yml#models/fixtures/repo_unit.yml#modules/git/tree_entry_gogit.go#modules/git/tree_gogit.go#routers/web/repo/actions/view.go#routers/web/repo/issue_comment.go#services/actions/workflow.go#services/doctor/actions_test.go#services/pull/comment.go#services/pull/pull.go#services/pull/temp_repo.go#templates/base/head_navbar.tmpl#templates/swagger/v1_json.tmpl#tests/integration/actions_schedule_test.go#tests/integration/git_lfs_ssh_test.go#tests/integration/pull_create_test.go#tests/integration/pull_merge_test.go#tests/sqlite.ini.tmpl#web_src/js/components/ContextPopup.vue
No description provided.