fix: merge error issue 在window下 Merge报错 #578 #584
Conversation
| @@ -127,3 +130,7 @@ func (bm *BucketManager) GetBucketID(ds Ds, name BucketName) (BucketId, error) { | |||
| return bucket.Id, nil | |||
| } | |||
| } | |||
|
|
|||
| func (bm *BucketManager) close() error { | |||
| return bm.fd.Close() | |||
There was a problem hiding this comment.
bucket.Meta can't be removed when we call os.RemoveAll on windows because it not closed.
| return db.View(func(tx *Tx) error { | ||
| return filesystem.CopyDir(db.opt.Dir, dir) | ||
| return db.View(func(tx *Tx) (err error) { | ||
| err = tx.db.flock.Unlock() |
There was a problem hiding this comment.
If we do not do these lines, backup of nutsdb will be blocked on windows. The reason is the files are locked.
| return db.View(func(tx *Tx) error { | ||
| return tarGZCompress(w, db.opt.Dir) | ||
| return db.View(func(tx *Tx) (err error) { | ||
| err = tx.db.flock.Unlock() |
There was a problem hiding this comment.
Same with above
| @@ -188,6 +211,11 @@ func (db *DB) release() error { | |||
| return err | |||
| } | |||
|
|
|||
| err = db.ActiveFile.Close() | |||
There was a problem hiding this comment.
I think we should release and close ActiveFile when we release a db.
| @@ -392,7 +424,14 @@ func (db *DB) doWrites() { | |||
|
|
|||
| // setActiveFile sets the ActiveFile (DataFile object). | |||
| func (db *DB) setActiveFile() (err error) { | |||
| activeFilePath := getDataPath(db.MaxFileID, db.opt.Dir) | |||
| if db.ActiveFile != nil { | |||
There was a problem hiding this comment.
Same with above. I think we also need close the previous ActiveFile
| for { | ||
| entry, err := f.readEntry(off) | ||
| if err != nil { | ||
| // whatever which logic branch it will choose, we will release the fd. | ||
| _ = f.release() | ||
| // _ = f.release() |
There was a problem hiding this comment.
The old release only something wrong. But I think we also release it after iteration.
| db.Index.list.delete(bucket) | ||
| } | ||
| } | ||
| // FIXME: it's useful? |
There was a problem hiding this comment.
PINNED: It seems that it's not useful and it will prevent the ci
| if err := db.ActiveFile.rwManager.Close(); err != nil { | ||
| db.mu.Unlock() | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
Close the current ActiveFile because we will use a new one.
| // FIXME: it's useful? | ||
| // func tarDecompress(dst string, src io.Reader) error { | ||
| // tarReader := tar.NewReader(src) | ||
|
|
||
| // for { | ||
| // header, err := tarReader.Next() | ||
| // if err == io.EOF { | ||
| // break | ||
| // } | ||
| // if err != nil { | ||
| // return err | ||
| // } | ||
|
|
||
| // path := filepath.Join(dst, header.Name) | ||
| // info := header.FileInfo() | ||
| // if info.IsDir() { | ||
| // if err = os.MkdirAll(path, info.Mode()); err != nil { | ||
| // return err | ||
| // } | ||
| // continue | ||
| // } | ||
|
|
||
| // file, err := os.OpenFile(filepath.Clean(path), os.O_CREATE|os.O_TRUNC|os.O_WRONLY, info.Mode()) | ||
| // if err != nil { | ||
| // return err | ||
| // } | ||
| // defer file.Close() | ||
| // _, err = io.Copy(file, tarReader) | ||
| // if err != nil { | ||
| // return err | ||
| // } | ||
| // } | ||
|
|
||
| // return nil | ||
| // } |
There was a problem hiding this comment.
PINNED, same with above
| @@ -658,6 +662,7 @@ func (tx *Tx) setStatusRunning() { | |||
| tx.status.Store(status) | |||
| } | |||
|
|
|||
| // FIXME: it's useful? | |||
| @@ -186,11 +186,12 @@ func TestTx_LPop(t *testing.T) { | |||
| runNutsDBTest(t, nil, func(t *testing.T, db *DB) { | |||
| txCreateBucket(t, db, DataStructureList, bucket, nil) | |||
| txPop(t, db, bucket, GetTestBytes(0), nil, ErrListNotFound, true) | |||
|
|
|||
| db.Close() | |||
There was a problem hiding this comment.
On windows, I can run the next case if we don't close db in here. The files will be occupated. Some other unit cases same with here.
This late PR is for fixing #578 .
The superficial reason of this issue is: some places we use
filepath.Cleanand some places not. If we usefilepath.Cleanwhen we callWithDir, nutsdb will work good coincidentally.In facts, there're more deeper reasons cause this reason. In one word, we don't close/release each file handler after it's used up. Windows is sensitive for these handlers. Of course, perhaps my understanding is incorrect.
Now I add more
xxx.Close()for file handlers.Another fact for approve my view above is many unit cases work bad on windows and some cases caused by the above
reason. Them work good after my fix for file handlers.Something more, for avoid non-standard handling of paths, I add a package named
nutsdbpathto solve paths.Something Confused Me
I saw that some methods call multiple
runNutsDBTestand the operation of creating bucket. Now I think we will throw errors if we do it. But there're many unit cases do these things.Advices for
runNutsDBTestThere're some unit cases which call multiple
runNutsDBTest, but the two calls are not associated. In this scene, it will work bad on windows. Thet.CleanUponly run when all cases finished, so it may not useful enough.Others
Some important things are mentions by comments in code or PR page. This PR may need a carefully review