Adding context.Context as parameter to Download.Start and Cancellation to the main Binary

[CodeLieutenant]

1. Extracting context.Context from the internals of the Download.Start() method as placing it as parameters allows fine graned control over the cancellation. This breaks backwards compatibility!

2. Cancellation to the program in main binary - when system interrupts the program in any way unfinished download should be removed from the system

Signed-off-by: Dusan Malusev dusan.998@outlook.com

[melbahja]

I think it will be better if we add Context to Download struct, So we can check for cancellation it in Init(), and GetInfo() and other methods for e.g:

ctx, cancel := context.WithCancel(context.Background())
dl, err := got.New(ctx, "http://speedtest.ftp.otenet.gr/files/test10Mb.db", "path")

// ...
dl = &got.Download{
   Context: ctx,
}

what do you think? I can make the change... (because of BC this will be in v0.2.0)

[CodeLieutenant]

I was thinking on this also, but it looked a little bit clunky, we'd need to add nil guard checks pretty much everywhere context was used, but it would be great to have context on got.Download, it allows for more granular control over cancellation

[melbahja]

Init() can set default Context to context.Background() if Download Context is nil? then when context in Download you can control cancellation everywhere without passing context as a argument to functions/methods.

[CodeLieutenant]

After a little bit of thinking about the problem, (this will make a huge change to structure), we can make Init method private, and just rely on got.New with config parameter. This will not allow for context to be null in the system and library API will be simpler

dl, err := got.New(got.Config{
Context: context.Background(),
Url: "http://speedtest.ftp.otenet.gr/files/test10Mb.db",
...
})
// Now dl can be interface and got.Download can be private
dl.Start()

By doing this, it will allow for more options in the future without breaking backwards compatibility, as more optional options can be added to the got.Config structure

Performance implications: This adds some cost to the performance but still network is the biggest bottleneck here

[CodeLieutenant]

Init() can set default Context to context.Background() if Download Context is nil? then when context in Download you can control cancellation everywhere without passing context as a argument to functions/methods.

Yes this is also the good option

[melbahja]

After a little bit of thinking about the problem, (this will make a huge change to structure), we can make Init method private, and just rely on got.New with config parameter. This will not allow for context to be null in the system and library API will be simpler

dl, err := got.New(got.Config{
Context: context.Background(),
Url: "http://speedtest.ftp.otenet.gr/files/test10Mb.db",
...
})
// Now dl can be interface and got.Download can be private
dl.Start()

By doing this, it will allow for more options in the future without breaking backwards compatibility, as more optional options can be added to the got.Config structure

Performance implications: This adds some cost to the performance but still network is the biggest bottleneck here

this is also a good idea...

func ApproachOne() {

    dl, err := got.New(ctx, "https://example.com", "/save/path")

    err := dl.Start()

}

func ApproachTwo() {

    g := got.New(got.Config{
        Context: context.Background(),
        Concurrency: 4, 
   // all other fields except URL and Dest
    })

    err := g.Download("https://example.com", "/save/path")
}

as you can see ApproachTwo you can download multiple files using Download() method, and one config for many files.

[melbahja]

It's good to simplify things now before too many people start using it.

[CodeLieutenant]

I have already implemented prototype for the ApproachOne, i will have it done by tomorrow, from there we can start upgrading it to ApproachTwo, since those two are simullar

[melbahja]

Cool thank you :)

[CodeLieutenant]

Cool thank you :)

Thank you for the awesome project

[CodeLieutenant]

@melbahja There is quite a lot of new changes for the new API, i have finished first part of this draft.

1. Start method: Now it does not depend on errgroup package (this package waits for all goroutines to finish before returning an error), when first error occurs (Download error or Interrupt error) it will immediately return it back to the caller. This makes program faster and correctly responds to cancel() from context

2. All types are extracted into separate file

3. got.New accepts two parameters, context.Context and got.Config, context inside got.Download is now private

4. Download.ChunkSize is also private and calculated, this will help when designing the multiple downloads, as ChunkSize can be removed from struct to avoid data races

5. Download struct is now composed with Info and Config, (for multiple downloads Info should be removed, it can cause data races)

6. Fix: When download is canceled, Start method returns ErrDownloadAborted as expected

[melbahja]

opened by mistake