Skip to content
This repository has been archived by the owner on Feb 7, 2024. It is now read-only.

Shell.add, file will be closed by shell.Shell automatically #230

Open
treasersimplifies opened this issue Dec 30, 2020 · 3 comments
Open
Labels
need/triage Needs initial labeling and prioritization

Comments

@treasersimplifies
Copy link

treasersimplifies commented Dec 30, 2020

I found in request.go:

func (r *Request) Send(c *http.Client) (*Response, error) {
    // snip
    if resp.StatusCode >= http.StatusBadRequest {
        // snip
        io.Copy(ioutil.Discard, resp.Body)
        resp.Body.Close()
    }
}

when developer use go-ipfs-api, they typically:

// var shells []*shell.Shell = ... connection to a lot of ipfs nodes..
localFile, err := os.Open(localfPath)
defer localFile.Close()
if err != nil {
    return nil, err
}
err = retry.Retry(func(attempt uint) error {
    response, err = shells[attempt].Add(localFile)
    if err != nil {
	return err
    }
    return nil
},
    strategy.Limit(limit),
)

So when fail on one shell.add, shell will close the file it pass, it's not good. The close of file should leave to developer.

@treasersimplifies treasersimplifies added the need/triage Needs initial labeling and prioritization label Dec 30, 2020
@welcome
Copy link

welcome bot commented Dec 30, 2020

Thank you for submitting your first issue to this repository! A maintainer will be here shortly to triage and review.
In the meantime, please double-check that you have provided all the necessary information to make this process easy! Any information that can help save additional round trips is useful! We currently aim to give initial feedback within two business days. If this does not happen, feel free to leave a comment.
Please keep an eye on how this issue will be labeled, as labels give an overview of priorities, assignments and additional actions requested by the maintainers:

  • "Priority" labels will show how urgent this is for the team.
  • "Status" labels will show if this is ready to be worked on, blocked, or in progress.
  • "Need" labels will indicate if additional input or analysis is required.

Finally, remember to use https://discuss.ipfs.io if you just need general support.

@Stebalien
Copy link
Member

That code closes the response body, not the request file.

However, go-ipfs-files closes the request file when it finishes reading it. This could be changed, but it would be a fair amount of work for little gain (we rely on this logic to close files as we traverse directory trees).

In general, the code above will never work as you'd need to seek back to the beginning of the file for each retry. The current behavior (an error) is significantly better than the bug you'd get if we didn't close the file (empty files).

@aschmahmann
Copy link
Contributor

@treasersimplifies is this issue a request for adding better documentation to the Add command and/or go-ipfs-files? Mutating file handles as you work with them seems pretty common place, but it may be worth pointing out that it's happening.

PRs welcome if you have wording/documentation suggestions here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
need/triage Needs initial labeling and prioritization
Projects
None yet
Development

No branches or pull requests

3 participants