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

return errors from cmds.Copy directly #64

Closed
wants to merge 1 commit into from

Conversation

whyrusleeping
Copy link
Member

Fixes the issue seen in ipfs/kubo#4683 But i'm not 100% sure this is the right way.

cc @kevina, you mentioned you could help look into cmds lib issues.

@ghost ghost assigned whyrusleeping Feb 14, 2018
@ghost ghost added the status/in-progress In progress label Feb 14, 2018
@codecov
Copy link

codecov bot commented Feb 14, 2018

Codecov Report

Merging #64 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
- Coverage      44%   43.95%   -0.05%     
==========================================
  Files          23       23              
  Lines        1925     1927       +2     
==========================================
  Hits          847      847              
- Misses        938      939       +1     
- Partials      140      141       +1
Impacted Files Coverage Δ
responseemitter.go 54.54% <0%> (-5.46%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1114463...9cd8024. Read the comment docs.

@kevina kevina self-requested a review February 14, 2018 06:14
@kevina kevina self-assigned this Feb 14, 2018
@kevina
Copy link
Contributor

kevina commented Feb 14, 2018

This might be a problem if multiple errors are emitted, but I am not sure yet. Right now I don't think we do it and it could be that emitting multiple errors was part of the original design, but only @keks can know for sure.

kevina added a commit to ipfs/kubo that referenced this pull request Feb 14, 2018
kevina added a commit to ipfs/kubo that referenced this pull request Feb 16, 2018
@Stebalien
Copy link
Member

As far as I can tell, this code is already doing what it's supposed to do. Response.Next() is supposed to determine that the value is actually an error and return it as an error.

@Stebalien
Copy link
Member

I believe the correct fix is #66. The other Response implementations already do this.

@kevina
Copy link
Contributor

kevina commented Feb 16, 2018

This changes the output of the repo gc command that does essentially emit two errors. Before t0087-repo-robust-gc.sh 48 - 'ipfs repo gc --stream-errors' should abort and report each error separately the output would be:

Error: could not retrieve links for QmSijovevteoY63Uj1uC5b8pkpDU5Jgyk2dYBqz3sMJUPc: merkledag: not found
Error: could not retrieve links for QmTbPEyrA1JyGUHFvmtx1FNZVzdBreMv8Hc8jV9sBRWhNA: The block referred to by 'QmTbPEyrA1JyGUHFvmtx1FNZVzdBreMv8Hc8jV9sBRWhNA' was not a valid merkledag node
Error: encountered errors during gc run
Error: garbage collection aborted: could not retrieve some links

Now it is just

Error: could not retrieve links for QmTbPEyrA1JyGUHFvmtx1FNZVzdBreMv8Hc8jV9sBRWhNA: The block referred to by 'QmTbPEyrA1JyGUHFvmtx1FNZVzdBreMv8Hc8jV9sBRWhNA' was not a valid merkledag node
Error: encountered errors during gc run

The last error not emitted is the error returned from the GC() function, which is concerning.

This needs to sorted out.

@Stebalien Stebalien closed this Feb 20, 2018
@ghost ghost removed the status/in-progress In progress label Feb 20, 2018
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.

3 participants