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

Plug edgecase for finding existing files #33

Merged
merged 2 commits into from
Dec 7, 2020

Conversation

xloem
Copy link
Contributor

@xloem xloem commented Dec 5, 2020

This changes the match detection code for D links so that when a file has been multiply uploaded due to some edge case, all uploads are compared to find the D record. Otherwise you get an issue where you keep uploading the same D records over and over and over.

This PR includes #32's fix for the rogue commit in #31.

This reverts commit 03abe75.

This was committed mistakenly, with a misunderstanding how findExist is used.
findExist does correctly verify its content.  There's no reason to tell the user only a hash is used.
@xloem
Copy link
Contributor Author

xloem commented Dec 5, 2020

This has a bug I've fixed locally. I'll reopen when it's tested.

@xloem xloem closed this Dec 5, 2020
@xloem
Copy link
Contributor Author

xloem commented Dec 5, 2020

Bug fixed. Should be good to merge.

@monkeylord
Copy link
Owner

Well, I see.
The first rejected promise makes promise.race rejected.

However, this will change the return of findExist.
How about making findExist return the first valid TX?

Cache.saveFileRecord(sha1, records)
return matchTX
return matchTXs
Copy link
Owner

Choose a reason for hiding this comment

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

return matchTXs[0]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue with returning one TX is that if it is not the one used by the most recent D record, it will never match.

logic.js Outdated
@@ -123,16 +123,19 @@ async function reduceFileDatum (fileDatum, address) {
API.log(`[+] Checking Exist Record`, API.logLevel.INFO)
for (var fileData of fileDatum) {
API.log(` - Checking ${fileData.dKey}`, API.logLevel.INFO)
var fileTX = await API.findExist(fileData.buf, fileData.mime)
Copy link
Owner

Choose a reason for hiding this comment

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

Then we don't need to change this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is changed so that all TX ids can be compared, when looking for D records. Not doing that is producing the edge cases that result in uploading repeatedly forever. But I see I failed to make the same change to findD, leaving more edge cases. I'll close this PR and reopen it when that change is made.

Copy link
Contributor Author

@xloem xloem Dec 6, 2020

Choose a reason for hiding this comment

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

Update: Actually, it looks good. The reason this is changed is so that all the TX ids can be compared with the most recent D record. If only one is compared, it reuploads again even if the D record has a different link with the same content. (I also had an additional bug where it would reupload forever; I haven't found that one, not sure how.)

@xloem
Copy link
Contributor Author

xloem commented Dec 6, 2020

Closing until I fix more edgecases similar to these, in findD.

@xloem xloem closed this Dec 6, 2020
@xloem xloem reopened this Dec 6, 2020
@xloem
Copy link
Contributor Author

xloem commented Dec 6, 2020

On review, the code looks good. I'll add a comment to each line comment.

@xloem
Copy link
Contributor Author

xloem commented Dec 7, 2020

would it work better to make a findExists function for returning an array, and keep findExist returning a single value?

@monkeylord
Copy link
Owner

would it work better to make a findExists function for returning an array, and keep findExist returning a single value?

good idea

@monkeylord monkeylord merged commit f94aba8 into monkeylord:master Dec 7, 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