Limit reading bytes instead of ReadAll #35928
Conversation
GiteaBot added the lgtm/need 2 
wxiaoguang force-pushed the fix-read-all branch from 16d228b to 21199e8 Compare wxiaoguang force-pushed the fix-read-all branch from 21199e8 to 4a4378d Compare 
| I would possible tend to create a single file listing these limits as constant's, so finding them becomes slightly more structured. |
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, |
| 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. |
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 |
GiteaBot added lgtm/need 1 | Doesn't block 1.25.2 release, no need to wait for this. |
delvh left a comment
delvh left a comment There was a problem hiding this comment.
Let's hope that 1MB is really large enough that no user ever encounters it.
If not, we can still adjust the limits.
| } | ||
| defer reader.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.
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.
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.
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.
GiteaBot added lgtm/done
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
ChristopherHX left a comment There was a problem hiding this comment.
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.
| } | ||
| defer reader.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.
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.
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? |
wxiaoguang changed the title 5 participants
No description provided.