fix: make number of concurrent downloads configurable

[Oppen]

Adds a new environment variable SHIORI_MAX_PAR_DL to limit the number
of concurrent downloads on the update command (and other, if necessary).
It defaults to the number of logical CPUs reported by the Go runtime.

Fixes #966

[Oppen]

I'm converting to draft to apply the change to a few other changes where it's relevant before merging. It'll be ready soon.

[Oppen]

I totally forgot about this. Now it's ready.

[Oppen]

I'm thinking of an improvement that could come later on, which is keeping around exactly the number of goroutines we need and sending tasks via channels. That should reduce both the (small) CPU overhead of setting the goroutine up and the (somewhat larger, 4kB per goroutine as a baseline) memory overhead of having too many goroutines lying around, even tho only a few are active at any given time.
To be clear, I would rather have this feat merged before, and maybe implement the other thing later.

[fmartingr]

I'm thinking of an improvement that could come later on, which is keeping around exactly the number of goroutines we need and sending tasks via channels. That should reduce both the (small) CPU overhead of setting the goroutine up and the (somewhat larger, 4kB per goroutine as a baseline) memory overhead of having too many goroutines lying around, even tho only a few are active at any given time. To be clear, I would rather have this feat merged before, and maybe implement the other thing later.

One thing I wanted to delve at some point is a background job system for shiori, were all these background task would be sent (download, import, export, etc) and being able to see the status of them easily rather than just firing goroutines and wait for them. Would be pretty useful for the UI/Integrations.

[fmartingr]

Thanks for the contribution @Oppen, just a question regarding the configuration.

[Monirzadeh]

Hi.
thanks for PR. maybe i miss undrtand this part.
you handle if Max_PAR_DL be 0 but what exacly happen if user not set Max_PAR_DL at all (or empty) are you sure it define as 0 by default?
can have a unittest for this?

[Oppen]

My understanding is that it defaults to whatever zero-value the type has, in this case integer 0:

noinit - do not initialize struct fields unless environment variables were provided. The default behavior is to deeply initialize all fields to their default (zero) value.
https://github.com/sethvargo/go-envconfig

Emphasis mine.

[Monirzadeh]

can have a unittest for that?
Set different value with environment variable and be sure set expected configuration cfg.Storage.MaxParDl.
Specifically for Max_PAR_DL=""
Max_PAR_DL=-1 , unset Max_PAR_DL etc.

[Oppen]

Does Go support manipulation of env vars for unit tests?

[Monirzadeh]

I think you can use this example
https://gobyexample.com/environment-variables
And
https://go-cloud-native.com/golang/test-environment-variables-in-go
And
https://beta.pkg.go.dev/testing@master#T.Setenv
Not forget to cleanup environment variable when the test done.

[Oppen]

I'll try on the weekend. I'm a bit wary of races testing like this tho, doesn't tests run in parallel?

[Oppen]

What I had in mind was something that acted more like mocking at the runtime level or something like that.

[fmartingr]

I'll try on the weekend. I'm a bit wary of races testing like this tho, doesn't tests run in parallel?

Only if you mark them with t.Parallel(), so it depends on the test.

[Monirzadeh]

Hi
Thanks for this PR. i left one comment that not clear to me and not bad if we have a unit test for that.
to avoid something like #981 happen again.