lnd
lnd copied to clipboard
lightningnetwork
Writefile transactional
Change Description
This PR is on top of #6723 which can be closed in favor of this one. This addresses issue #6720 to call fsync when writing to files.
Steps to Test
This is quite difficult to test as a manual integration test. I added some unit test cases that can be reviewed.
Pull Request Checklist
Testing
- [x] Your PR passes all CI checks.
- [X] Tests covering the positive and negative (error paths) are included.
Code Style and Documentation
- [X] The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
- [X] Commits follow the Ideal Git Commit Structure.
- [x] There is a change description in the release notes, or
[skip ci]in the commit message for small changes.
@gregwebs I saw #6769 but I haven't found any other build that the linter fails on the same line. Linter would pass if we use the value and not the pointer, but I might want to check what's causing it.
I added some changes so itest could work 👇🏼
diff --git a/io/io_test.go b/io/io_test.go
index 97f8a7aa3..2d6047500 100644
--- a/io/io_test.go
+++ b/io/io_test.go
@@ -54,16 +54,16 @@ func TestWriteFileTransactional(t *testing.T) {
// Changing the permission to read-only will cause the write to fail
// transactional first writes to a tmp file and then renames
- tmp_name := filename + ".tmp"
- _, err := os.Stat(tmp_name)
+ tmpName := filename + ".tmp"
+ _, err := os.Stat(tmpName)
if err == nil {
- t.Fatalf("WriteFileTransactional tmp exists %s: %v", tmp_name, err)
+ t.Fatalf("WriteFileTransactional tmp exists %s: %v", tmpName, err)
}
- if fh, err := os.Create(tmp_name); err != nil {
+ if fh, err := os.Create(tmpName); err != nil {
t.Fatalf("Error setting up read-only %s: %v", filename, err)
fh.Close()
}
- if err := os.Chmod(tmp_name, 0444); err != nil {
+ if err := os.Chmod(tmpName, 0444); err != nil {
t.Fatalf("Error changing to read-only %s: %v", filename, err)
}
@@ -78,9 +78,9 @@ func TestWriteFileTransactional(t *testing.T) {
}
ensureFileContents(t, filename, data1)
- _, err = os.Stat(tmp_name)
+ _, err = os.Stat(tmpName)
if err == nil {
- t.Fatalf("WriteFileTransactional tmp exists %s: %v", tmp_name, err)
+ t.Fatalf("WriteFileTransactional tmp exists %s: %v", tmpName, err)
}
}
diff --git a/io/write.go b/io/write.go
index bc9f3588a..2918e8283 100644
--- a/io/write.go
+++ b/io/write.go
@@ -33,7 +33,7 @@ func WriteFileTransactional(file string, fileBytes []byte, perm fs.FileMode) err
err := os.Rename(tmpName, file)
if err != nil {
// Ignore this error and return the prior error
- // This may be unecessary but shouldn't cause a problem
+ // This may be unnecessary but shouldn't cause a problem
_ = os.Remove(file)
}
return err
diff --git a/lnrpc/chainrpc/chainnotifier_server.go b/lnrpc/chainrpc/chainnotifier_server.go
index d1de7b988..e1e206c02 100644
--- a/lnrpc/chainrpc/chainnotifier_server.go
+++ b/lnrpc/chainrpc/chainnotifier_server.go
@@ -7,7 +7,6 @@ import (
"bytes"
"context"
"errors"
- "os"
"path/filepath"
"sync"
diff --git a/lnrpc/invoicesrpc/invoices_server.go b/lnrpc/invoicesrpc/invoices_server.go
index ca5d811d9..ff46d95bf 100644
--- a/lnrpc/invoicesrpc/invoices_server.go
+++ b/lnrpc/invoicesrpc/invoices_server.go
@@ -6,7 +6,6 @@ package invoicesrpc
import (
"context"
"fmt"
- "os"
"path/filepath"
"github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
diff --git a/lnrpc/signrpc/signer_server.go b/lnrpc/signrpc/signer_server.go
index 45b88d89c..278668e51 100644
--- a/lnrpc/signrpc/signer_server.go
+++ b/lnrpc/signrpc/signer_server.go
@@ -8,7 +8,6 @@ import (
"context"
"crypto/sha256"
"fmt"
- "os"
"path/filepath"
"github.com/btcsuite/btcd/btcec/v2"
diff --git a/lnrpc/walletrpc/walletkit_server.go b/lnrpc/walletrpc/walletkit_server.go
index ad6d4a0c7..5f2786910 100644
--- a/lnrpc/walletrpc/walletkit_server.go
+++ b/lnrpc/walletrpc/walletkit_server.go
@@ -10,7 +10,6 @@ import (
"errors"
"fmt"
"math"
- "os"
"path/filepath"
"time"
@gregwebs I saw #6769 but I haven't found any other build that the linter fails on the same line. Linter would pass if we use the value and not the pointer, but I might want to check what's causing it.
Oh, it fails on me on my local machine for master as well. I haven't seen a single open PR that is passing so thought it must be affecting others as well. It is a very strange failure: I cannot explain it.
I added some changes so itest could work 👇🏼
Awesome, thank you, I will apply that!
Is there a reason to go against best practice and use a bool, or do you also prefer an enum type?
Either way, it seems clear to me that a transactional write would be preferable for this project. I have a commit here that uses an existing library that also maintains permissions and works on windows: https://github.com/gregwebs/lnd/commit/72886227e04b608fd564874495f1cab6a1e6af51
Is there a reason to go against best practice and use a bool, or do you also prefer an enum type?
An enum is fine
Either way, it seems clear to me that a transactional write would be preferable for this project. I have a commit here that uses an existing library that also maintains permissions and works on windows: https://github.com/gregwebs/lnd/commit/72886227e04b608fd564874495f1cab6a1e6af51
In a different PR sure, but I don't think we should be adding dependencies if we don't need to
I lost my bandwidth to follow through on this, perhaps @mauricepoirrier would like to. I will comment back here if I am able to pick it back up.
@gregwebs, remember to re-request review from reviewers when ready
@gregwebs if you need some help, I can pick up this issue.
That would be great if you can take over the issue.
@gregwebs, remember to re-request review from reviewers when ready
@gregwebs, remember to re-request review from reviewers when ready
@gregwebs in order to have write access I opened a new PR #7192. I squashed the commits but still part of release notes :)
Any review is welcome! cc: @Crypt-iQ
