code review 91830043: leveldb: close the log file when the DB is closed, so t...

leveldb: close the log file when the DB is closed, so that we don't
lose any buffered writes when the WriteOptions.Sync field is false.

Update issue 13.

[nigeltao, 2014-04-26 19:14:39]

Hello bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/leveldb-go

[bradfitz, 2014-04-26 19:16:45]

LGTM



On Sat, Apr 26, 2014 at 12:14 PM, <nigeltao@golang.org> wrote:

> Reviewers: bradfitz,
>
> Message:
> Hello bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/leveldb-go
>
>
> Description:
> leveldb: close the log file when the DB is closed, so that we don't
> lose any buffered writes when the WriteOptions.Sync field is false.
>
> Update issue 13.
>
> Please review this at https://codereview.appspot.com/91830043/
>
> Affected files (+63, -0 lines):
>   M leveldb/leveldb.go
>   M leveldb/leveldb_test.go
>
>
> Index: leveldb/leveldb.go
> ===================================================================
> --- a/leveldb/leveldb.go
> +++ b/leveldb/leveldb.go
> @@ -186,6 +186,8 @@
>         if d.fileLock == nil {
>                 return err
>         }
> +       err = firstError(err, d.log.Close())
> +       err = firstError(err, d.logFile.Close())
>         err = firstError(err, d.fileLock.Close())
>         d.fileLock = nil
>         return err
> Index: leveldb/leveldb_test.go
> ===================================================================
> --- a/leveldb/leveldb_test.go
> +++ b/leveldb/leveldb_test.go
> @@ -421,3 +421,64 @@
>                 t.Fatalf("db Close: %v", err)
>         }
>  }
> +
> +func TestOpenCloseOpenClose(t *testing.T) {
> +       opts := &db.Options{
> +               FileSystem: memfs.New(),
> +       }
> +
> +       for _, length := range []int{-1, 0, 1, 1000, 10000, 100000} {
> +               dirname := strconv.Itoa(length)
> +               got, xxx := []byte(nil), ""
> +               if length >= 0 {
> +                       xxx = strings.Repeat("x", length)
> +               }
> +
> +               d0, err := Open(dirname, opts)
> +               if err != nil {
> +                       t.Errorf("length=%d: Open #0: %v", length, err)
> +                       continue
> +               }
> +               if length >= 0 {
> +                       err = d0.Set([]byte("key"), []byte(xxx), nil)
> +                       if err != nil {
> +                               t.Errorf("length=%d: Set: %v", length, err)
> +                               continue
> +                       }
> +               }
> +               err = d0.Close()
> +               if err != nil {
> +                       t.Errorf("length=%d: Close #0: %v", length, err)
> +                       continue
> +               }
> +
> +               // TODO: make the second Open recover (without a fatal
> "corrupt log
> +               // file" error) even if the d0 database was not closed but
> the xxx
> +               // value is large enough to write a partial record.
> Writing to the
> +               // database should not corrupt it even if the writer
> process was
> +               // killed part-way through.
> +
> +               d1, err := Open(dirname, opts)
> +               if err != nil {
> +                       t.Errorf("length=%d: Open #1: %v", length, err)
> +                       continue
> +               }
> +               if length >= 0 {
> +                       got, err = d1.Get([]byte("key"), nil)
> +                       if err != nil {
> +                               t.Errorf("length=%d: Get: %v", length, err)
> +                               continue
> +                       }
> +               }
> +               err = d1.Close()
> +               if err != nil {
> +                       t.Errorf("length=%d: Close #1: %v", length, err)
> +                       continue
> +               }
> +
> +               if length >= 0 && string(got) != xxx {
> +                       t.Errorf("length=%d: got value differs from set
> value", length)
> +                       continue
> +               }
> +       }
> +}
>
>
>

[nigeltao, 2014-04-26 19:26:35]

*** Submitted as
https://code.google.com/p/leveldb-go/source/detail?r=48507a759c8d ***

leveldb: close the log file when the DB is closed, so that we don't
lose any buffered writes when the WriteOptions.Sync field is false.

Update issue 13.

LGTM=bradfitz
R=bradfitz
CC=golang-codereviews
https://codereview.appspot.com/91830043