No delay at all isn't ok. notify sometimes creates multiple events for a single change, at least those should be aggregated.

@imsodin What is the minimum delay that would be fine in your opinion? Would 100ms be ok?

I just don't want == 0, as that means multiple triggered scans by design if there are duplicate events. Even back to back events in our tests are reported in <1ms, so a 10ms lower limit should be very save and that's already below human perception.

I opened the original #7859 and was going to add a comment, but @imsodin said it already. I originally suggested that fsWatcherDelayS could be changed to either fractional seconds or milliseconds so as to be able to set it very low, but leaving it as seconds and making it so a value of "0" actually translates to something like "10ms" is absolutely fine for the case I explained, requires (as far as I can see) minimal source changes, and no config migration.

I've given this a shot. fsWatcherDelayS stays as it was and keeps using seconds, but it gets converted to milliseconds later in the code. When set to 0, a hard floor of 10ms is used instead. Please let me know if there are any flaws in the code and/or if this can be done better or simpler.

Keeping in mind once again that I have limited experience with Go and even less with this codebase, so apologies if I'm not making any sense: I thought implementing that strategy could be limited to something like this:

  func notifyTimeout(eventDelayS int) time.Duration {
          shortDelayS := 10
          shortDelayMultiplicator := 6
          longDelayS := 60
          longDelayTimeout := time.Duration(1) * time.Minute
          if eventDelayS < shortDelayS {
                  return time.Duration(eventDelayS*shortDelayMultiplicator) * time.Second
          }
          if eventDelayS < longDelayS {
                  return longDelayTimeout
          }
-         return time.Duration(eventDelayS) * time.Second
+         if (eventDelayS === 0) {
+                 // Clamp at 10ms minimum
+                 return time.Duration(10) * time.Millisecond
+         } else {
+                 return time.Duration(eventDelayS) * time.Second
+         }
}

So basically just special-casing eventDelayS == 0.

I've modified the code to follow the suggestion by @bard, because it seems much simpler, while doing the same job.

@imsodin

Lets not change the semantics that 0 means default.

Is it really bad to just allow 0? With the old code, trying to use 0 simply resets the value to the default 10. It's not like you can have 0 actually set there in config.xml. With this commit, 0 would simply mean 0 (or 10ms in reality), and negative numbers would still reset the value to 10. What do you think?

What I basically mean that 0 in this case is not the same as 0 with e.g. maxFolderConcurrency, where the number of 0 is accepted, while the actual value is dynamic or something like that.

Is it really bad to just allow 0?

Maybe. Zero values and defaults are a minefield (if you are interested, some thoughts on that and other things are here: https://github.com/syncthing/syncthing/wiki/Configuration). Maybe this is fine, maybe it isn't - it doesn't matter because the actually simple and least surprising option is keeping the current behaviour, just allowing fractions of a second as a value. Simple in terms of just needing some special casing somewhere in the code is not a good criterium.

Simple in terms of just needing some special casing somewhere in the code is not a good criterium.

From my understanding, the value in question is meant to capture user intent, not a software contract.

Theoretically, if the value were part of a contract between different modules of syncthing, or between syncthing and other programs, I'd agree with you — if one module/program promises 500 X's and delivers 510 X's, it makes the job of the module/program on the other side more complicated. ("Theoretically" because, when dealing with time, the case isn't that clear-cut — distributed systems have little choice but to accept and plan for variability, or be fragile.)

When it comes to user intent, I think it's important to stay aware of what the number (or the string, or any vehicle of meaning) is supposed to encode. If I tap my phone's screen, and my fingertip ends up touching just one pixel below a hyperlink, where nothing clickable is present, is the browser's code doing me a favor and "special-casing" by activating the hyperlink, or is it fulfilling its contract with me, which consists in interpreting my intent correctly?

Ideally, user configuration values take intent into account as well. If I say, "sync at 10 seconds", I'm not really saying "sync at 10000ms" — 9500ms or 10500ms or anything in between captures my intent just as correctly. If I want to say "sync immediately", to say "sync at 0 seconds" captures my intent fine, too.

All IMHO, of course.

(Perhaps the crux here is just that a user value shouldn't wait too long and until it's too deep into the stack before intent is extracted from it?)

I incorrectly assumed the 0 value is documented to be default - it isn't. Nevertheless it is the case for some other config options and it has been the case for this one previously - I want to keep that behaviour. I don't want e.g. some script that used 0 to break. I don't understand why we are discussing this - there is a simple, backwards compatible solution, and I haven't heard any arguments against it yet.

@imsodin if you mean allowing fractions of a second as a value, it was also my first suggestion in the original issue, and the argument against it was the cost of migrating configuration.

#7859 (comment)

It might be mostly free to change the type to float and keep the seconds unit.

It might be mostly free to change the type to float and keep the seconds unit.

I have tried to convert the value from int to float, and it does seem to work, but don't really have confidence regarding my modifications. I had to edit quite some code related to usage reporting, including migration, and I don't know whether it works or not.

Also, if I set the type to float in folderconfiguration.proto, it automatically uses float32, but there is no sortFloats32 that could be used instead of sortInts in https://github.com/syncthing/syncthing/pull/7864/files#diff-dbcda7c99edad4fd4234bbc68e60511d0751bc10ea9a0e96618cfbca6eb73f95. I found https://gist.github.com/harihar/83670077ec532858ba1c390a65d3d91f and applied the solution from the stackoverflow answer linked there, but I'm not sure if this is the right way. On a side note, trying to use float64 in folderconfiguration.proto results in complaints about it being not defined.

When it comes to generating proto itself, I used the precompiled binary mentioned in https://forum.syncthing.net/t/exec-protoc-executable-file-not-found-in-path/17307, so I also hope that the generated code is fine.

#7859 (comment)

It might be mostly free to change the type to float and keep the seconds unit.

Ah, fair enough, I didn't realize that addressed the objection, I thought the migration would be necessary regardless.

float64 is spelled double in protobuf, though I don't think the precision is essential. For usage reporting I think the simplest (though not 100% correct) solution is to simply round the value to an integer for now -- possibly rounding up, so that 0.1s gets reported as 1s (which is notably different from the default and from zero).

Looks like this needs a go mod tidy with go1.17 to build due to the new handling of indirect dependencies there.

float64 is spelled double in protobuf, though I don't think the precision is essential. For usage reporting I think the simplest (though not 100% correct) solution is to simply round the value to an integer for now -- possibly rounding up, so that 0.1s gets reported as 1s (which is notably different from the default and from zero).

Just to confirm, I should revert the usage reporting changes, keeping the value as int, and only do a conversion from float32 to int with rounding up beforehand. Is this correct?

That sounds reasonable to me, yes.

The few non-integer values we have in the config today are double, so perhaps use that instead of float.

🤖 beep boop

I'm going to close this pull request as it has been idle for more than 90 days.

This is not a rejection, merely a removal from the list of active pull requests that is periodically reviewed by humans. The pull request can be reopened when there is new activity, and merged once any remaining issues are resolved.