save: Perform write process safe #3273

JoeKar · 2024-04-28T15:10:39Z

The target situation shall be, that micro checks if the file to be stored already exists and if so it shall work with a temporary file first, before the target file is overwritten respective the temporary file renamed to the target file.
Possible symlinks pointing to the target file will be resolved, before the save takes place. This shall guarantee that this symlink isn't renamed by the new approach.

TODOs:

Properly handle all of the irregular file types and character devices etc.pp.
Simplify the chosen overwriteFile() interface (see: save: Perform write process safe #3273 (comment))
util.EscapePath(b.AbsPath) does not uniquely encode the file path (see: https://github.com/zyedidia/micro/pull/3273/files#r1599137940)
b.Backup() executed asynchronously (from backupThread) is accessing the line array without locking. (see: https://github.com/zyedidia/micro/pull/3273/files#r1599137940)
Decouple the backup step from the target file write (see: https://github.com/zyedidia/micro/pull/3273/files#r1600795305)
Remove broken backup (see: https://github.com/zyedidia/micro/pull/3273/files#r1599076040)
Rework save & backup trigger (see: save: Perform write process safe #3273 (comment))

Fixes #1916
Fixes #3148
Fixes #3196

internal/buffer/save.go

internal/buffer/buffer.go

internal/buffer/save.go

internal/util/util.go

internal/buffer/save.go

internal/buffer/buffer.go

internal/action/bindings.go

internal/buffer/buffer.go

cmd/micro/micro.go

internal/util/util.go

internal/buffer/save.go

JoeKar · 2024-05-16T20:04:43Z

Still a lot of rework left, but thank your for your time reviewing this! 👍

JoeKar · 2024-05-26T14:04:54Z

I did a lot of (commit) refactoring, but still I'm not fully happy with it.
The backup approach is still messy (duplicated code, no single point of truth, etc.), but currently I'm running out of ideas.

internal/buffer/buffer.go

internal/buffer/save.go

internal/util/util.go

dmaluka · 2024-05-27T01:57:07Z

I did a lot of (commit) refactoring, but still I'm not fully happy with it. The backup approach is still messy (duplicated code, no single point of truth, etc.), but currently I'm running out of ideas.

IIUC, basically, all these troubles are due to the need to avoid circular dependencies between packages?

If so, it seems we could avoid these troubles by implementing overwrite & backup stuff as a part of the config package. OTOH, that sounds like an abuse of the config package: it is supposed to implement stuff related specifically to configuration, not stuff like buffer backups.

So maybe instead, simply provide separate implementations of overwrite & backup stuff for different kinds of files, in both buffer and config packages (especially since, as I noted in another comment, it seems better to have separate implementations of Overwrite() anyway). Maybe make them implement some common interface (if there is a need for that).

A very rough idea:

type FileWriter interface {
	Overwrite(name string) error
	BackupName(name string) string
	KeepBackup() bool
}

and a common Write() implementation using this interface:

func Write(name string, fwriter FileWriter) error

Or maybe even Write() implementations need to be different for different files.

P.S. Side note: maybe let's name it SafeWrite()?

JoeKar · 2024-06-01T14:59:02Z

Yes, I know dc701a1 doesn't really belong to the PR in the first place, but I touched some WriteFile's anyway.
It was not a big deal to do this cleanup.

internal/buffer/backup.go

If the new URL encoded path is found then it has precedence over the '%' escaped path. In case none of both is found the new URL approach is used.

internal/buffer/save.go

internal/util/util.go

This is fully handled within the buffers `save` domain.

…cess SafeWrite() will create a temporary intermediate file.

…le()`

- extract the open logic into `openFile()` and return a `wrappedFile` - extract the closing logic into `Close()` and make a method of `wrappedFile` - rename `writeFile()` into `Write()` and make a method of `wrappedFile` This allows to use the split parts alone while keeping overwriteFile() as simple interface to use all in a row.

As advantage we don't need to synchonize them any longer and don't need further insufficient lock mechanisms.

Now the main go routine takes care of the backup synchronization.

…ails

dmaluka · 2024-12-29T15:14:50Z

internal/buffer/save.go

@@ -153,26 +197,27 @@ func (b *Buffer) saveToFile(filename string, withSudo bool, autoSave bool) error
 		}
 	}

+	filename, _ = util.ReplaceHome(filename)


One more thought: it doesn't seem quite ok that we just ignore the error here...

I assume we should treat absFilename, _ := filepath.Abs(filename) then the same and care about the error.

Furthermore...

// Update the last time this file was updated after saving defer func() { b.ModTime, _ = util.GetModTime(filename) err = b.Serialize() }()

...should be moved behind the actual successful save request or even to the end of the saveToFile() function. Because otherwise the modification time will be updated and the buffer serialized without the safe and successful save.

dmaluka · 2025-01-01T13:22:57Z

internal/buffer/save.go

+			return wrappedFile{}, err
+		}
+	} else {
+		writeCloser, err = os.OpenFile(name, os.O_WRONLY|os.O_CREATE, util.FileMode)


Similarly to util.SafeWrite(), for newly created files: we successfully create an empty file, then we fail to write this file, we report the error but keep this empty file around?

dmaluka · 2025-01-01T16:51:54Z

~/.config/micro/buffers/history is still not covered by util.SafeWrite().

So when micro at exit tries to save the command history and fails to do that, it fails silently [*], and when I start micro next time, I see Error loading history:EOF, since the history file is there but is empty.

[*] Formally, it does report the error, but in such a way that no one can see it.

JoeKar · 2025-01-05T12:34:47Z

~/.config/micro/buffers/history is still not covered by util.SafeWrite().

So when micro at exit tries to save the command history and fails to do that, it fails silently [*], and when I start micro next time, I see Error loading history:EOF, since the history file is there but is empty.

[*] Formally, it does report the error, but in such a way that no one can see it.

Thanks for spotting.
It was out of scope so far...at least for me. I will take care of it in the next days.

JoeKar requested a review from dmaluka April 28, 2024 15:10

dmaluka reviewed Apr 28, 2024

View reviewed changes

internal/buffer/save.go Outdated Show resolved Hide resolved

JoeKar force-pushed the fix/save-atomically branch 2 times, most recently from 7d663d8 to afdc7a1 Compare April 29, 2024 21:46

dmaluka reviewed Apr 29, 2024

View reviewed changes

internal/buffer/save.go Outdated Show resolved Hide resolved

dmaluka reviewed Apr 30, 2024

View reviewed changes

internal/buffer/save.go Outdated Show resolved Hide resolved

internal/buffer/save.go Show resolved Hide resolved

internal/buffer/save.go Outdated Show resolved Hide resolved

internal/buffer/save.go Outdated Show resolved Hide resolved

JoeKar marked this pull request as draft April 30, 2024 18:46

JoeKar force-pushed the fix/save-atomically branch 3 times, most recently from 1ea6aaf to 3510fcc Compare May 12, 2024 19:47

JoeKar marked this pull request as ready for review May 12, 2024 19:49

JoeKar commented May 13, 2024

View reviewed changes

internal/buffer/buffer.go Outdated Show resolved Hide resolved

dmaluka reviewed May 13, 2024

View reviewed changes

dmaluka reviewed May 15, 2024

View reviewed changes

internal/util/util.go Outdated Show resolved Hide resolved

dmaluka reviewed May 15, 2024

View reviewed changes

internal/buffer/save.go Outdated Show resolved Hide resolved

JoeKar marked this pull request as draft May 23, 2024 18:39

JoeKar force-pushed the fix/save-atomically branch from 3510fcc to 97a4e83 Compare May 25, 2024 15:20

dmaluka reviewed May 27, 2024

View reviewed changes

internal/buffer/buffer.go Show resolved Hide resolved

internal/buffer/save.go Outdated Show resolved Hide resolved

internal/buffer/save.go Outdated Show resolved Hide resolved

internal/util/util.go Show resolved Hide resolved

JoeKar force-pushed the fix/save-atomically branch from ea4176c to 474f98d Compare June 1, 2024 14:56

JoeKar marked this pull request as ready for review June 2, 2024 19:36

dmaluka reviewed Jun 4, 2024

View reviewed changes

internal/buffer/backup.go Outdated Show resolved Hide resolved

JoeKar force-pushed the fix/save-atomically branch from 474f98d to 8c6835c Compare June 4, 2024 18:29

dmaluka mentioned this pull request Aug 7, 2024

Micro saves files non-atomically #3148

Open

X-Ryl669 mentioned this pull request Aug 27, 2024

New release needed ? #3444

Open

JoeKar force-pushed the fix/save-atomically branch 2 times, most recently from 42e896f to b92a6cd Compare September 3, 2024 16:30

JoeKar added 5 commits December 28, 2024 14:15

clean: Inform about all failed write steps

66b0a6e

clean: Remove some unneeded filepath.Join() calls

a13b568

util: Improve and rename EscapePath() to DetermineEscapePath()

70b49c3

If the new URL encoded path is found then it has precedence over the '%' escaped path. In case none of both is found the new URL approach is used.

util: Generalize the file mode of 0666 with util.FileMode

b15fd90

ioutil: Remove deprecated functions where possible

201852e

JoeKar force-pushed the fix/save-atomically branch from 02c5bfd to edc190d Compare December 28, 2024 13:15

dmaluka reviewed Dec 28, 2024

View reviewed changes

internal/buffer/save.go Outdated Show resolved Hide resolved

internal/util/util.go Outdated Show resolved Hide resolved

JoeKar force-pushed the fix/save-atomically branch from edc190d to 3f0e289 Compare December 28, 2024 19:48

JoeKar added 17 commits December 29, 2024 13:52

save: Perform write process safe

cf8caab

actions: Don't overwrite the buffers Path

94e7506

This is fully handled within the buffers `save` domain.

util: Provide AppendBackupSuffix() for further transformations

b86bbe7

backup: Perform write process safe

8593ca1

util: Provide SafeWrite() to generalize the internal file write pro…

0d7d95f

…cess SafeWrite() will create a temporary intermediate file.

serialize: Perform write process safe

b82d9c4

bindings: Perform write process safe

6f0ba17

settings: Perform write process safe

5a37163

save: Merge overwrite() into overwriteFile() and extract `writeFi…

010bf3e

…le()`

micro: Generalize exit behavior

893e52c

micro: Provide recovery of settings.json & bindings.json

1a8fc50

backup: Rearrange and extend BackupMsg

4a403d0

buffer: Remove superfluous backupTime

1aede74

save+backup: Process the save & backup with a sequential channel

72aeadb

As advantage we don't need to synchonize them any longer and don't need further insufficient lock mechanisms.

backup: Clear the requested backup upon completion notification

c7904a4

Now the main go routine takes care of the backup synchronization.

save+util: Provide a meaningful error message for safe (over-)write f…

fd97574

…ails

JoeKar force-pushed the fix/save-atomically branch from 3f0e289 to fd97574 Compare December 29, 2024 12:55

dmaluka reviewed Dec 29, 2024

View reviewed changes

dmaluka reviewed Jan 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

save: Perform write process safe #3273

save: Perform write process safe #3273

JoeKar commented Apr 28, 2024 •

edited

Loading

JoeKar commented May 16, 2024

JoeKar commented May 26, 2024

dmaluka commented May 27, 2024 •

edited

Loading

JoeKar commented Jun 1, 2024

dmaluka Dec 29, 2024

JoeKar Dec 29, 2024 •

edited

Loading

dmaluka Jan 1, 2025

dmaluka commented Jan 1, 2025

JoeKar commented Jan 5, 2025

save: Perform write process safe #3273

Are you sure you want to change the base?

save: Perform write process safe #3273

Conversation

JoeKar commented Apr 28, 2024 • edited Loading

JoeKar commented May 16, 2024

JoeKar commented May 26, 2024

dmaluka commented May 27, 2024 • edited Loading

JoeKar commented Jun 1, 2024

dmaluka Dec 29, 2024

Choose a reason for hiding this comment

JoeKar Dec 29, 2024 • edited Loading

Choose a reason for hiding this comment

dmaluka Jan 1, 2025

Choose a reason for hiding this comment

dmaluka commented Jan 1, 2025

JoeKar commented Jan 5, 2025

JoeKar commented Apr 28, 2024 •

edited

Loading

dmaluka commented May 27, 2024 •

edited

Loading

JoeKar Dec 29, 2024 •

edited

Loading