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

Fixes re-downloading data corrupt for the same url #514

Merged
merged 10 commits into from
Mar 17, 2020

Conversation

zhongwuzw
Copy link
Contributor

Fixes #489 , we use cacheKey to get the task, actually it would mix up data if previous task cancelled but download delegate still not finished , and next task with the same url started at the same time. This is the race condition for the same url downloading, we can keep the weak reference to task and get it in session delegate.

@bolsinga
Copy link
Contributor

bolsinga commented Jul 9, 2019

Please add a test.

@garrettmoon
Copy link
Collaborator

This is a really great fix! Would you mind adding a test as @bolsinga suggested?

@zhongwuzw zhongwuzw force-pushed the fix_url_re-download branch from 844712c to 5d58a20 Compare July 10, 2019 12:48
@zhongwuzw
Copy link
Contributor Author

@bolsinga @garrettmoon Please review again. :)

@bolsinga
Copy link
Contributor

Thanks for the test!

@garrettmoon
Copy link
Collaborator

Can you add an entry to CHANGELOG.md for this?

@ghost
Copy link

ghost commented Jul 11, 2019

🚫 CI failed with log

@garrettmoon
Copy link
Collaborator

Mind rebasing against master? I think I fixed a flakey test.

@ghost
Copy link

ghost commented Jul 25, 2019

🚫 CI failed with log

@zhongwuzw
Copy link
Contributor Author

@garrettmoon Tests failed like below frequently, I think we may need to add a command like rm -rf ~/Library/Developer/Xcode/DerivedData/ before build project in CI?

image

@jparise
Copy link
Collaborator

jparise commented Mar 10, 2020

Sorry for the long delay @zhongwuzw. The CI tests should now be fixed. If you could rebase again, we can do a final review of your change.

@zhongwuzw
Copy link
Contributor Author

@jparise Done.

@jparise jparise requested a review from garrettmoon March 11, 2020 02:38
@jparise jparise requested a review from bolsinga March 11, 2020 02:44
@zhongwuzw zhongwuzw force-pushed the fix_url_re-download branch from 00fe21b to 60d2405 Compare March 12, 2020 01:56
Copy link
Collaborator

@jparise jparise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good, @zhongwuzw! Just a few more bits of feedback.

@zhongwuzw
Copy link
Contributor Author

This is looking really good, @zhongwuzw! Just a few more bits of feedback.

Done.

Copy link
Collaborator

@jparise jparise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! We can merge this once the other folks who have left comments (@garrettmoon, @bolsinga) give it a 👍, too.

@garrettmoon
Copy link
Collaborator

This looks great, thank you so much for the fix!

Copy link
Contributor

@bolsinga bolsinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accepted; good fix; opens up some good questions on how to test the framework better going forward!

@garrettmoon garrettmoon merged commit c1d7f3a into pinterest:master Mar 17, 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.

The data received does not correctly match the corresponding PINRemoteImageDownloadTask
5 participants