-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
download: Add support for http long polling #3418
download: Add support for http long polling #3418
Conversation
af6301a
to
f3c1655
Compare
6a0a9e3
to
022f06c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got some more questions 🙃 Please bear with me
func (d *Downloader) doStart(ctx context.Context) { | ||
ctx, cancel := context.WithCancel(context.Background()) | ||
|
||
d.wg.Add(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 Is there ever more than one entry in this waitgroup? That is, are there multiple bundles downloaded (or long-polled) in parallel? If so, I guess I don't see [edit] it...
If we don't need it for multiple entries, I suspect there might be some room for simplification here... we've got so many synchronisation stuff here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We wait for the goroutine that performs the actual download so that it can finish calling the callback function provided by the plugins. For example, in the bundle plugin, after we stop a bundle's downloader, we delete its status object. So if we don't wait for the goroutine to finish, it's possible that plugin may have already deleted the bundle's status object which would result in a panic in the callback function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I see. What I still don't understand is if that waitgroup ever has more than 1 group member...? But I'll have another closer look tomorrow.
022f06c
to
d0e92af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The waitgroup use is just a nitpick. LGTM.
5cdc000
to
b1a7002
Compare
Earlier the downloader package only supported the http short polling technique where the client sends periodic requests to the server to fetch bundles. A drawback of this method is that a low polling frequency could add unnecessary burden on the server and network. This commit adds support for http long polling which helps to minimize server/network resource usage and also reduces the delay in delivery of updates to the client. Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
b1a7002
to
72ec49d
Compare
Earlier the downloader package only supported the http
short polling technique where the client sends periodic
requests to the server to fetch bundles. A drawback of this
method is that a low polling frequency could add unnecessary
burden on the server and network.
This commit adds support for http long polling which helps
to minimize server/network resource usage and also reduces
the delay in delivery of updates to the client.
Signed-off-by: Ashutosh Narkar anarkar4387@gmail.com