cavaliergopher / grab

A download manager package for Go
BSD 3-Clause "New" or "Revised" License
1.38k stars 151 forks source link

Synchronized access to Response.transfer and Response.bytesResumed #28

Closed cmaglie closed 6 years ago

cmaglie commented 6 years ago

I've been experiencing "race condition detected" errors during tests, I'm displaying a progress bar during download (using another goroutine), that's why those fields needs to be protected for concurrent access.

cmaglie commented 6 years ago

Mmmmmhhh... talked too early, after some more testing, there are still some problems (the goroutine that does the "progress bar" is blocked until the download completes, but this happens only for very long downloads).

cmaglie commented 6 years ago

Ok I've added another commit to the PR, this solved the issue for good this time.

cavaliercoder commented 6 years ago

Thanks for taking the time to submit this. It's a pretty complicated issue, and you've done well to solve it. I'm not sure if your approach is the way I wan't to move forward though - and it's entirely my fault.

When I introduced the transfer type I thought I had this really neat, reusable, thread-safe type that could be independently open sourced. I neglected to note that the reference to this new type was not thread-safe. I'd rather solve this issue by undoing my poor design choice and creating much more clearly defined ownership of memory.

The previous model implied two access rules:

The problem is that Response.transfer is being read in another goroutine (not its thread-safe children), before the transfer starts.

I hope this might be solved with one or two lines of code.

cavaliercoder commented 6 years ago

Declined in favor of #29. Apologies and gratitude!

cmaglie commented 6 years ago

No problem, and thanks for doing a proper fix!