itchio / butler

🎩 Command-line itch.io helper
MIT License
745 stars 52 forks source link

'Apply' command doesn't close patchReader #193

Closed alexdmtr closed 5 years ago

alexdmtr commented 5 years ago

While investigating how to integrate butler into another project, unit tests reveal that the 'apply' command does not close the handle to the patch file.

Specifically, I believe that in in cmd/apply/apply.go

patchReader, err := eos.Open(patch, option.WithConsumer(comm.NewStateConsumer()))

patchReader is never closed.

I've attached the following test script; running Test_Apply reproduces the issue. The code creates a test environment (NewConfig) as a temporary folder with old and new "versions" to test patch creation and application. The error occurs in Clean, when the temporary folder cannot be deleted (since patch.pwr is still open by the process)

package patch

import (
    "io/ioutil"
    "os"
    "path/filepath"
    "testing"
    "time"

    "github.com/itchio/butler/cmd/apply"
    "github.com/itchio/butler/cmd/diff"
    "github.com/itchio/wharf/pwr"
    "github.com/stretchr/testify/assert"
)

func Test_Apply(t *testing.T) {
    config := NewConfig()
    defer config.Clean()

    // Create the patch
    err := diff.Do(&diff.Params{
        // Target is the old version of the data
        Target: config.OldDir(),
        // Source is the new version of the data
        Source: config.NewDir(),
        // Patch is where to write the patch
        Patch: config.PatchFile(),
        Compression: pwr.CompressionSettings{
            Algorithm: pwr.CompressionAlgorithm_NONE,
            Quality:   1,
        },
        Verify: false,
    })
    assert.NoError(t, err)

    // Apply the patch
    err = apply.Do(&apply.Params{
        Patch:         config.PatchFile(),
        Target:        config.OldDir(),
        Output:        config.OutputDir(),
        SignaturePath: config.PatchSig(),
        InPlace:       config.OutputDir() == config.NewDir(),
    })
    assert.NoError(t, err)
}

type testConfig struct {
    mainDir string
}

func (t *testConfig) OldDir() string {
    return filepath.Join(t.mainDir, "old")
}

func (t *testConfig) NewDir() string {
    return filepath.Join(t.mainDir, "new")
}

func (t *testConfig) OutputDir() string {
    return filepath.Join(t.mainDir, "output")

}

func (t *testConfig) OldSig() string {
    return filepath.Join(t.mainDir, "old.sig")
}

func (t *testConfig) PatchFile() string {
    return filepath.Join(t.mainDir, "patch.pwr")

}

func (t *testConfig) PatchSig() string {
    return filepath.Join(t.mainDir, "patch.pwr.sig")

}

// Clean deletes all directories and files created for the testConfig
func (t *testConfig) Clean() {
    err := os.RemoveAll(t.mainDir)
    if err != nil {
        // I've tried waiting, in case butler does any async cleanup -- doesn't appear to be the case
        time.Sleep(100 * time.Millisecond)

        err = os.RemoveAll(t.mainDir)

        if err != nil {
            panic(err)
        }
    }
}

/* NewConfig creates a test configuration in form of a reference to a temp directory containing
 and old directory, a new directory and space for the patch
 */

func NewConfig() *testConfig {
    var config testConfig
    var err error
    // Create main directory
    config.mainDir, err = ioutil.TempDir("", "patch_test")
    if err != nil {
        panic(err)
    }

    // Create old directory
    if err = os.MkdirAll(config.OldDir(), 0700); err != nil {
        panic(err)
    }

    // Create and close old file
    oldFile, err := os.Create(filepath.Join(config.OldDir(), "file.txt"))
    if err != nil {
        panic(err)
    }
    if _, err = oldFile.WriteString("Hello world!"); err != nil {
        panic(err)
    }
    if err = oldFile.Close(); err != nil {
        panic(err)
    }

    // Create new directory
    if err = os.MkdirAll(config.NewDir(), 0700); err != nil {
        panic(err)
    }

    // Create and close new file
    newFile, err := os.Create(filepath.Join(config.NewDir(), "file.txt"))
    if err != nil {
        panic(err)
    }
    if _, err = newFile.WriteString("Hello there!"); err != nil {
        panic(err)
    }

    if err = newFile.Close(); err != nil {
        panic(err)
    }

    return &config
}
fasterthanlime commented 5 years ago

That's correct - packages under cmd/ map directly to butler's CLI commands, and are not designed to be used at the API level - after calling butler apply, butler exits normally and all file handles are closed.

Using the ApplyContext struct directly lets you have more control over what happens (and, as you noticed, the consumer of that API is in charge of closing the reader).