Re: [PATCH v4 2/2] gitk: synchronize config write
From: Paul Mackerras <hidden>
Date: 2016-06-15 23:03:56
On Mon, Nov 10, 2014 at 12:20:02AM +0200, Max Kirillov wrote:
If several gitk instances are closed simultaneously, safestuff procedure can run at the same time, resulting in a conflict which may cause losing of some of the instance's changes, failing the saving operation or even corrupting the configuration file. This can happen, for example, at user session closing, or at group closing of all instances of an application which is possible in some desktop environments. To avoid this, make sure that only one saving operation is in progress. It is guarded by existance of $config_file_tmp file. Both creating the file and moving it to $config_file are atomic operations, so it should be reliable. Reading does not need to be syncronized, because moving is atomic operation, and the $config_file always refers to full and correct file. But, if there is a stale $config_file_tmp file, report it at gitk start. If such file is detected at saving, just abort the saving, because this is how gitk used to handle errors while saving. Signed-off-by: Max Kirillov <redacted>
The idea looks good; I have a couple of comments on the patch. First, 50 tries over 5 seconds seems a bit excessive to me, wouldn't (say) 20 tries be enough? Is the 50 the result of some analysis?
+ error_popup "Probably there is stale $config_file_tmp file; config saving is going to fail. Check if it is being used by any existing gitk process and remove it otherwise"
I would word this as "There appears to be a stale $config_file_tmp file, which will prevent gitk from saving its configuration on exit. Please remove it if it is not being used by any existing gitk process."
quoted hunk ↗ jump to hunk
@@ -2811,11 +2824,16 @@ proc savestuff {w} { if {$stuffsaved} return if {![winfo viewable .]} return + set remove_tmp 0 catch { - if {[file exists $config_file_tmp]} { - file delete -force $config_file_tmp + set try_count 0 + while {[catch {set f [open $config_file_tmp {WRONLY CREAT EXCL}]}]} { + if {[incr try_count] > 50} { + error "Unable to write config file: $config_file_tmp exists" + } + after 100 } - set f [open $config_file_tmp w] + set remove_tmp 1 if {$::tcl_platform(platform) eq {windows}} { file attributes $config_file_tmp -hidden true }@@ -2878,6 +2896,14 @@ proc savestuff {w} { puts $f "}" close $f file rename -force $config_file_tmp $config_file + set remove_tmp 0 + return "" + } err + if {$err ne ""} { + puts "Error saving config: $err"
I would suggest checking the return from the catch statement, like
this:
if {[catch {
...
file rename -force $config_file_tmp $config_file
} err]} {
puts "Error saving config: $err"
if {$remove_tmp} {
file delete -force $config_file_tmp
}
}
rather than doing a return inside the catch.
Paul.