melbahja · 2020-08-15T13:37:19Z

Adding support for multiple file downloads and `Context`

Related PRs: #15 and #16

The New API:

Multiple file download

g := got.New()

// Optional
// g.Client = your custom http client

err := g.Download("http://lcoalhost/file.ext", "/path/to/save")

Multiple files with context:

g := got.NewWithContext(context.Background())

// file 1
err := g.Download("http://lcoalhost/file.ext", "/path/to/save1")

// file 2
err := g.Download("http://lcoalhost/file2.ext", "/path/to/save2")

// Download config.
err := g.Do(&got.Download{
		URL:       "http://lcoalhost/file2.ext",
		Dest:      "/path/to/save2",
		Concurrency: 10,
})

Download Single file:

  ctx := context.Background()

	dl := got.NewDownload(ctx,"http://lcoalhost/file.ext", "/path/to/save")

	// Init
	if err := dl.Init(); err != nil {
		// err
	}

	// Start download
	if err := dl.Start(); err != nil {
		// err
	}

CLI

Single file:

got https://example.com/file.mp4

Multiple files:

got https://example.com/file.mp4 https://example.com/file2.mp4

Multiple save to specific dir:

got --dir /path/to/dir https://example.com/file.mp4 https://example.com/file2.mp4

From stdin:

cat file.txt | got --dir /dir

From file:

got --bf file.txt --dir /dir

Waiting for your feedback:
@suzaku @malusev998 @poldi1405 @xurwxj

Adding context and support for multi file downloads. change: `New` func now returns new *Got instead of *Download add: `NewWithContext` func. add: `NewDownload` returns new *Download change: Progress methods now is a part of the download add: Context add: `RunProgress` method to run ProgressFunc rm: `Progress` removed

CodeLieutenant · 2020-08-15T15:09:45Z

I have implemented a little bit different approach, simpler to multiple downloads,
Comment

API:

dl, err := got.New(context.Background(), got.Config { })
// Check error
err = dl.Download(url, savePath)
// Check error


// Multiple downloads can be done like this

go func() { dl.Download(...) }
// Many times ...

This requires a little bit more change for the pull request #16 (Combine methods Start() and others into Downalod).
There are a couple of problems related to the data races with this approach, Download.Dest and Download.Url must be removed from the structure to avoid data races.
Then single file downloads and multiple file downloads will have same API, the only difference would be multiple downloads will run in their separate go routines.
If you can merge #16 to this draft, this will give us simple API for the library and really good commandline application

melbahja · 2020-08-15T16:21:51Z

In this draft every file have an independent Download{} item to avoid data races

You made a good point there about errgroup
The user can set custom ChunkSize, if not set then ChunkSize calculated in Init
Now Progress and info removed. because every file is a independent Download item.

I guess I have to remove errgroup and replaced with channels like what you did?

CodeLieutenant · 2020-08-15T16:51:32Z

Yeah, errgroup should be removed, it is slower as it waits for all goroutines to finish before handling error.
For ChunkSize in commandline it's not needed for multiple downloads, it would be better just to calculate the ChunkSize

Could you make a full example for your version of the API, so we can check how it works and make more proposals

melbahja · 2020-08-15T17:20:20Z

You can check examples in:
https://github.com/melbahja/got/blob/v0_2/got_test.go#L60
and
https://github.com/melbahja/got/blob/v0_2/download_test.go#L260

CodeLieutenant · 2020-08-15T17:35:08Z

You can check examples in:
https://github.com/melbahja/got/blob/v0_2/got_test.go#L60
and
https://github.com/melbahja/got/blob/v0_2/download_test.go#L260

What about got.Config ?

Example:

d := got.New(got.Config{ })
d := got.NewWithContext(ctx, got.Config{})

melbahja · 2020-08-15T17:45:04Z

we can use Config to share options with Download items like Interval, Concurrency and ProgressFunc

CodeLieutenant · 2020-08-15T17:48:53Z

we can use Config to share options with Download items like Interval, Concurrency and ProgressFunc

Yeah, that would be great,
PS. Should http.Client be exposed ?

melbahja · 2020-08-15T17:54:57Z

yes http.Client so users can set there custom http client

melbahja · 2020-08-15T17:55:38Z

I will remove errgroup from dl

CodeLieutenant · 2020-08-15T18:02:37Z

New API looks really simple, now I see, there is no need for got.Config

PS. I and my friend are making PKGBUILD for AUR

melbahja · 2020-08-15T18:11:42Z

cool 😎

CodeLieutenant · 2020-08-15T18:13:14Z

cool 😎

When 0.2 will be released?

melbahja · 2020-08-15T18:41:55Z

probably tonight

suzaku · 2020-08-15T22:30:42Z

Yeah, errgroup should be removed, it is slower as it waits for all goroutines to finish before handling error.

This is simply not true, as soon as the first error occurs, the context is canceled and all context-aware goroutines get a chance to stop.

For ChunkSize in commandline it's not needed for multiple downloads, it would be better just to calculate the ChunkSize

Could you make a full example for your version of the API, so we can check how it works and make more proposals

melbahja · 2020-08-15T23:16:10Z

@suzaku is errgroup faster than the channel approach?
if you can see the code here:
https://github.com/golang/sync/blob/master/errgroup/errgroup.go

I think channels faster?

melbahja · 2020-08-16T00:04:35Z

@suzaku waiting for your feedback before merge.

suzaku · 2020-08-16T01:13:10Z

@suzaku waiting for your feedback before merge.

It doesn't really matter here. When thinking about performance, we need to pinpoint the bottleneck. I don't think the bottleneck of downloading is the errgroup vs. channel choice. I think we should pick which one is simpler to use and less error-prone.

CodeLieutenant · 2020-08-16T10:24:21Z

errgroup is the bottleneck,

errgroup.Wait()
Wait blocks until all function calls from the Go method have returned, then returns the first non-nil error (if any) from them.

This is unnecessary usage of resources, when network error happens or any other error, download should be stop (maybe layer down the line we can implement some kind of recovery mechanism).
For now it is ok to stop download of the file when first error occurs. So errgroup and channels dont do the same work (different behaviors).

melbahja · 2020-08-16T17:20:01Z

Merged thank you for your help ❤️

melbahja added 7 commits August 15, 2020 14:10

rm: progress remove and methods moved to Download.

3de4283

request funcs moved to got.go

7b663da

add: sync pool && Download method moved to Download

7866184

tests

e08ec4b

cli: handle piped stdin and batch file and multiple download args

ad9329f

rm tmp.

b210e4d

melbahja added 2 commits August 15, 2020 18:42

handle signal, and add dir flag

6eefa9f

add: error msg

c58c7a9

replace errgroup with channels

1d699c2

CodeLieutenant approved these changes Aug 15, 2020

View reviewed changes

melbahja added 2 commits August 15, 2020 22:58

rm: errgroup using channels instead.

949ff41

fmt and use one defer func in Start

037656e

melbahja added 3 commits August 16, 2020 00:09

improve chunk size calculation.

76f694f

improve cli tool

5b66243

add: default file name var

d142d32

fmt code.

98d5912

fix: use TotalSize in content tests

e3620cc

melbahja marked this pull request as ready for review August 16, 2020 00:04

fix: go vet errs

12a13de

melbahja added 9 commits August 16, 2020 11:49

Improve default concurrency calculation and reset chunk pool item.

97dd67a

improve cli output

872796b

update readme.

b136c2c

fix: typos

853a4de

fix: typos

2b5d10e

add: example

14e3b75

add github actions.

b6e5a71

add: race flag

410c983

fix: test for windows

43c714a

melbahja merged commit 7aade22 into master Aug 16, 2020

melbahja deleted the v0_2 branch August 16, 2020 17:20

Navigation Menu

v0.2 - #17

v0.2#17
melbahja merged 27 commits into
masterfrom
v0_2

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Uh oh!

Adding support for multiple file downloads and Context

The New API:

Multiple file download

Download Single file:

CLI

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Adding support for multiple file downloads and `Context`