Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 4 commits into from

Conversation

CodeLieutenant
Copy link
Contributor

  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

Extracting context.Context from the internals of the
Download.Start() method as placing it as parameters
allows fine graned control over the cancelation

Signed-off-by: Dusan Malusev <dusan.998@outlook.com>
@melbahja
Copy link
Owner

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
Copy link
Contributor Author

CodeLieutenant commented Aug 13, 2020

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

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

@CodeLieutenant
Copy link
Contributor Author

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
Copy link
Owner

Cool thank you :)

@CodeLieutenant
Copy link
Contributor Author

Cool thank you :)

Thank you for the awesome project

Dusan Malusev added 3 commits August 14, 2020 01:31
Signed-off-by: Dusan Malusev <dusan.998@outlook.com>
Signed-off-by: Dusan Malusev <dusan.998@outlook.com>
Signed-off-by: Dusan Malusev <dusan.998@outlook.com>
@CodeLieutenant
Copy link
Contributor Author

@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 melbahja mentioned this pull request Aug 15, 2020
Merged
@melbahja melbahja reopened this Aug 16, 2020
@melbahja
Copy link
Owner

melbahja commented Aug 16, 2020

opened by mistake

@melbahja melbahja closed this Aug 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants