Basically, if we don't care about the sync status of the file we should
not tag someone else out of sync because they don't have the latest
version. This solves *my* "Syncing - 100%" scenario at least.
The reason this happens seems to be like this, in my situation. I have
three devices, connected in a "line": A-B-C. A is a Mac and litters
.DS_Store files everywhere. I've ignored these, but some escaped into
the folders before I did so. I've also ignored them on B and C but at
different stages. B was flagging C as out of sync, because at the point
the ignores were introduced C had a lower version of .DS_Store than A.
Now none of them are sending updates about it any more since it's
ignored...
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3981
Other routines use atomics, hence even if we are under a lock, we should
too.
We might atomically store with
Not sure how it happens, but it's between lines
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3974
After this change,
- Symlinks on Windows are always unsupported. Sorry.
- Symlinks are always enabled on other platforms. They are just a small
file like anything else. There is no need to special case them. If you
don't want to sync some symlinks, ignore them.
- The protocol doesn't differentiate between different "types" of
symlinks. If that distinction ever does become relevant the individual
devices can figure it out by looking at the destination when they
create the link.
It's backwards compatible in that all the old symlink types are still
understood to be symlinks, and the new SYMLINK type is equivalent to the
old SYMLINK_UNKNOWN which was always a valid way to do it.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3962
LGTM: AudriusButkevicius
Instead of just immediately dropping the event if the subscription isn't
ready to receive it, give it 15 ms to catch up. The value 15 ms is
grabbed out of thin air - it just seems reasonable to me.
The timer juggling makes the event send pretty much exactly twice as
slow as it was before, but we're still under a microsecond. I think it's
negligible compared to whatever event that just happened that we're
interested in logging (usually a file operation of some kind).
benchmark old ns/op new ns/op delta
BenchmarkBufferedSub-8 475 950 +100.00%
benchmark old allocs new allocs delta
BenchmarkBufferedSub-8 4 4 +0.00%
benchmark old bytes new bytes delta
BenchmarkBufferedSub-8 104 117 +12.50%
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3960
Instead of
[I6KAH] 19:05:56 INFO: Single thread hash performance is 359 MB/s using minio/sha256-simd (354 MB/s using crypto/sha256).
it now says
[I6KAH] 19:06:16 INFO: Single thread SHA256 performance is 359 MB/s using minio/sha256-simd (354 MB/s using crypto/sha256).
[I6KAH] 19:06:17 INFO: Actual hashing performance is 299.01 MB/s
which is more informative. This is also the number it reports in usage
reporting.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3918
Can't do what I did, as the rolling function is not the same as the
non-rolling one. Instead this uses an improved version of the rolling
adler32 to accomplish the same thing. (PR filed on upstream, so should
be able to use that directly in the future.)
The rolling version of adler32 is just a wrapper around the standard
hash/adler32 when used in a non-rolling fashion, but it's inefficient as
it allocates a new hash instance for every Write(). This uses the
default version instead in the block hasher, and adds a test to verify
the result is the same as they were before. It reduces allocations by
88% and increases speed about 5%.
benchmark old ns/op new ns/op delta
BenchmarkHashFile-8 64434698 61303647 -4.86%
benchmark old MB/s new MB/s speedup
BenchmarkHashFile-8 276.65 290.78 1.05x
benchmark old allocs new allocs delta
BenchmarkHashFile-8 1238 150 -87.88%
benchmark old bytes new bytes delta
BenchmarkHashFile-8 17877363 49292 -99.72%
Syncthing adds some hidden files when a folder is added, but there is currently
no equivalent cleanup procedure. This change is conservative as not to
accidentally cause data loss.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3874
On Windows we would descend into SYMLINKD type links when we scanned
them successfully, as we would return nil from the walk function and the
filepath.Walk iterator apparently thought it OK to descend into the
symlinked directory.
With this change we always return filepath.SkipDir no matter what.
Tested on Windows 10 as admin, does what it should.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3875
Also tweaks the proto definitions:
- [packed=false] on the block_indexes field to retain compat with
v0.14.16 and earlier.
- Uses the vendored protobuf package in include paths.
And, "build.go setup" will install the vendored protoc-gen-gogofast.
This should ensure that a proto rebuild isn't so dependent on whatever
version of the compiler and package the developer has installed...
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3864
The protobuf encoder now produces packed arrays for things like []int32,
which is actually correct according to the proto3 spec. However
Syncthing v0.14.16 and earlier doesn't support this. This reverts the
encoding change, but keeps the updated decoder so that we are both more
compatible with other proto3 implementations and can move to the updated
encoder in the future.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3856
Since we anyway need the folderConfig for this I'm skipping the copying
of all it's attributes that rwfolder did and just keeping the original
around instead.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3825
This adds support for AES_256_GCM_SHA384 (in there since Go 1.5, a bit
of a shame we missed it) and ChaCha20-Poly1305 (if built with Go 1.8;
ignored on older Gos).
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3822
The test for the error string is fragile, and the error string changed
in Go 1.8 so the relevant part is no longer a prefix. This covers it
with a test though, so it should be fine in the future as well.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3818
Instead, trust (and test) that the temp file has appropriate permissions
from the start. The only place where this changes our behavior is for
ignores which go from 0644 to 0600. I'm OK with that.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3756
This changes the "seen" map that we're anyway keeping around to track
the modtimes of loaded files instead. When doing a Load() we check that
1) the file we are loading is in the modtime set, and 2) that none of
the files in the modtime set have changed modtimes. If that's the case
we do a quick return without parsing anything or clearing the cache.
This required adding two one seconds sleeps in the tests to make sure
the modtimes were updated when we expect cache reloads, because I'm on a
crappy filesystem with one second timestamp granularity. That also
proves it works...
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3754
Fsyncing the file has a small performance penalty and seems unnecessary. The
file will be fsynced anyway, when the changes are commited to the database.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3749
This makes the device ID a real type that can be used in the protobuf
schema. That avoids the juggling back and forth from []byte in a bunch
of places and simplifies the code.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3695
This adds autodetection of the fastest hashing library on startup, thus
handling the performance regression. It also adds an environment
variable to control the selection, STHASHING=standard (Go standard
library version, avoids SIGILL crash when the minio library has bugs on
odd CPUs), STHASHING=minio (to force using the minio version) or unset
for the default autodetection.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3617
When the GUI/API is bound to localhost, we enforce that the Host header
looks like localhost. This can be disabled by setting
insecureSkipHostCheck in the GUI config.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3558
When files that were previously marked as deleted became ignored, we
used to do nothing at all. This changes that behavior to set the Invalid
bit (that we should rename to Ignored). This then becomes an update to
other devices that they should not trust our knowledge about the file in
question.
Read this diff without whitespace...
Tested by
- creating a bunch of files on s1
- letting them sync to s2
- shutting down s2
- deleting the files on s1 and rescanning
- adding the files to .stignore on s1 and rescanning
- starting up s2 and letting it sync
- observing the files are not deleted on s2, and it considers itself up
to date.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3557
We used to consider deleted files & directories 128 bytes large. After
the delta indexes change a bug slipped in where deleted files would be
weighted according to their old non-deleted size. Both ways are
incorrect (but the latest change made it worse), as if there are more
files deleted than remaining data in the repo the needSize can be
greater than the globalSize, resulting in a negative completion
percentage.
This change makes it so that deleted items are zero bytes large, which
makes more sense. Instead we expose the number of files that we need to
delete as a separate field in the Completion() result, and hack the
percentage down to 95% complete if it was 100% complete but we need to
delete files. This latter part is sort of ugly, but necessary to give
the user some sort of feedback.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3556
We previously set the mtime on the temp file, and then renamed it to the
real path. Unfortunately that means we'd save the real timestamp under
the under the temp name ".syncthing.foo.tmp" when the actual file that
we will look up on the next scan is "foo". This moves the Chtimes later,
ensuring that it gets recorded correctly under the right name.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3519
These are no longer required with Go 1.7. Change made by removing the
functions, doing a global s/osutil.Remove/os.Remove/.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3514
So there were some issues here. The main problem was that
model.Close(deviceID) was overloaded to mean "the connection was closed
by the protocol layer" and "i want to close this connection". That meant
it could get called twice - once *to* close the connection and then once
more when the connection *was* closed.
After this refactor there is instead a Closed(conn) method that is the
callback. I didn't need to change the parameter in the end, but I think
it's clearer what it means when it takes the connection that was closed
instead of a device ID. To close a connection, the new close(deviceID)
method is used instead, which only closes the underlying connection and
leaves the cleanup to the Closed() callback.
I also changed how we do connection switching. Instead of the connection
service calling close and then adding the connection, it just adds the
new connection. The model knows that it already has a connection and
makes sure to close and clean out that one before adding the new
connection.
To make sure to sequence this properly I added a new map of channels
that get created on connection add and closed by Closed(), so that
AddConnection() can do the close and wait for the cleanup to happen
before proceeding.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3490
Furthermore:
1. Cleans configs received, migrates them as we receive them.
2. Clears indexes of devices we no longer share the folder with
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3478
It seems that it would be impossible to drop down to relay after establishing a direct connection
Also, we should not drop the existing connection until after we've passed the validation steps,
and it seems it's being dropped in two places unnecesserily at the moment.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3480
This adds a new nanoseconds field to the FileInfo, populates it during
scans and sets the non-truncated time in Chtimes calls.
The actual file modification time is defined as modified_s seconds +
modified_ns nanoseconds. It's expected that the modified_ns field is <=
1e9 (that is, all whole seconds should go in the modified_s field) but
not really enforced. Given that it's an int32 the timestamp can be
adjusted += ~2.9 seconds by the modified_ns field...
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3431
This adds a config to enable debug functions on the API server, which is
by default disabled. When enabled, the /rest/debug things become
available and become available without requiring a CSRF token (although
authentication is required if configured).
We also add a new endpoint /rest/debug/cpuprof?duration=15s (with the
duration being configurable, defaulting to 30s). This runs a CPU profile
for the duration and returns it as a file. It sets headers so that a
browser will save the file with an informative name.
The same is done for heap profiles, /rest/debug/heapprof, which does not
take any parameters.
The purpose of this is that any user can enable debugging under
advanced, then point their browser to the endpoint above and get a file
that contains a CPU or heap profile we can use, with the filename
telling us what version and architecture the profile is from.
On the command line, this becomes
curl -O -J http://localhost:8082/rest/debug/cpuprof?duration=5s
curl: Saved to filename
'syncthing-cpu-darwin-amd64-v0.14.3+4-g935bcc0-110307.pprof'
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3467
Otherwise if the file grows during scanning the block list will be out
of sync with the stated size and things get confused. We could fixup the
size afterwards based on the block list, but then we might see other
inconsistencies as the mtime should have changed to reflect the new size
etc. Better stick to the original state and let the next scan pick up
the change.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3442
We could have a file to sync with permissions rw------- but we'd create
the temp file with rw-rw-rw- minus umask, usually rw-r--r--. This
potentially exposes private data while the file is being synced.
Similarly, when ignorePerms was set and we were reusing a temp files we
would set the permissions to rw-r--r-- explicitly, potentially
overriding a strict umask that would otherwise have had the file be
rw-------.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3437
The previous commit loosened the locking around database updates.
Apparently that was not fine - what happens is that parallell updates
to the same file for different devices stomp on each others updates to
the global index, leaving it missing one of the two devices.
This lets us add message types in the future, for authentication or
other purposes, without completely breaking old clients. I see this as
similar behavior to adding fields to messages - newer clients must
simple be aware that older ones may ignore the message and act
accordingly.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3390
This slightly changes the interface used for committing configuration
changes. The two parts are now:
- VerifyConfiguration, which runs synchronously and locked, and can
abort the config change. These callbacks shouldn't *do* anything
apart from looking at the config changes and saying yes or no. No
change from previously.
- CommitConfiguration, which runs asynchronously (one goroutine per
call) *after* replacing the config and releasing any locks. Returning
false from these methods sets the "requires restart" flag, which now
lives in the config.Wrapper.
This should be deadlock free as the CommitConfiguration calls can take
as long as they like and can wait for locks to be released when they
need to tweak things. I think this should be safe compared to before as
the CommitConfiguration calls were always made from a random background
goroutine (typically one from the HTTP server), so it was always
concurrent with everything else anyway.
Hence the CommitResponse type is gone, instead you get an error back on
verification failure only, and need to explicitly check
w.RequiresRestart() afterwards if you care.
As an added bonus this fixes a bug where we would reset the "requires
restart" indicator if a config that did not require restart was saved,
even if we already were in the requires-restart state.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3386
A random "instance ID" is generated on each start of the local discovery
service. The instance ID is included in the announcement. When we see a
new instance ID we treat is a new device and respond with an
announcement of our own. Hence devices get to know each other quickly on
restart.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3385
This changes the BEP protocol to use protocol buffer serialization
instead of XDR, and therefore also the database format. The local
discovery protocol is also updated to be protocol buffer format.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3276
LGTM: AudriusButkevicius
This is a supplement patch to commit a58f69b which only fixed global
discovery. This patch adds the missing parts for the local discovery.
If the listen address scheme is set to tcp4:// or tcp6:// and no
explicit host is specified, an address should not be considered if the
source address does not match this scheme.
This prevents invalid URIs like tcp4://<IPv6 address>:<port> or tcp6://<IPv4
address>:<port> for local discovery.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3380
This contains the following behavioral changes:
- Duplicate folder IDs is now fatal during startup
- Invalid folder flags in the ClusterConfig is fatal for the connection
(this will go away soon with the proto changes, as we won't have any
unknown flags any more then)
- Empty path is a folder error reported at runtime
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3370
Events API consumers rely on being able to detect that events were skipped
by the fact that the event ID has increased by more than 1. This is
documented, and is absolutely necessary when trying to maintain a local
model of Syncthing's state.
With the introduction of LocalChangeDetected, which is not exposed to the
Events API, this contract was broken.
This commit introduces separate concepts of a "Global ID" and a
"Subscription ID". The Global ID of an event is unique across all
subscriptions. The Subscription ID is local to a particular subscription,
and always increments by 1. They are both exposed over the Events API, but
the Subscription ID uses the key "id" for backwards compatibility, and
the "?since=xx" parameter refers to the Subscription ID (making the Global
ID for information only).
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3351
LGTM: calmh
Also fixes an issue where the discovery cache call would only return the
newest cache entry for a given device instead of the merged addresses
from all cache entries (which is more useful).
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3344
The various path cleaning operations done in in cleanedPath() removes
it, so we make sure it's added again at the end. This makes adding the
slash in prepare() unnecessary, but keep it anyway for display purposes
(people looking at the config).
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3342
While attempting to fix#2782 I thought the problem was the
CheckFolderHealth method, so I cleaned it up. That turned out not to be
the case, but I think this is better anyhow.
It also moves the "create folder and marker if the folder was empty in
the index" code to StartFolder where I think it makes better sense.
This is covered by a number of existing tests.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3343
This adds a metric for "committed items" to the database instance that I
use in the test code, and a couple of tests that ensure that scans that
don't change anything also don't commit anything.
There was a case in the scanner where we set the invalid bit on files
that are ignored, even though they were already ignored and had the
invalid bit set. I had assumed this would result in an extra database
commit, but it was in fact filtered out by the Set... Anyway, I think we
can save some work on not pushing that change to the Set at all.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3298
This is in preparation for future changes, but also improves the
handling when talking to pre-v0.13 clients. It breaks out the Hello
message and magic from the rest of the protocol implementation, with the
intention that this small part of the protocol will survive future
changes.
To enable this, and future testing, the new ExchangeHello function takes
an interface that can be implemented by future Hello versions and
returns a version indendent result type. It correctly detects pre-v0.13
protocols and returns a "too old" error message which gets logged to the
user at warning level:
[I6KAH] 09:21:36 WARNING: Connecting to [...]:
the remote device speaks an older version of the protocol (v0.12) not
compatible with this version
Conversely, something entirely unknown will generate:
[I6KAH] 09:40:27 WARNING: Connecting to [...]:
the remote device speaks an unknown (newer?) version of the protocol
The intention is that in future iterations the Hello exchange will
succeed on at least one side and ExchangeHello will return the actual
data from the Hello together with ErrTooOld and an even more precise
message can be generated.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3289
1. For the same internal port we ask for the same external port on all devices. This can be a problem if one device speaks over two protocols.
2. Always add a nil address even if we managed to get external address of the gateway, just because the gateway might be in DMZ behind another gateway.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3196
The intention for this package is to provide a combination of the
security of crypto/rand and the convenience of math/rand. It should be
the first choice of random data unless ultimate performance is required
and the usage is provably irrelevant from a security standpoint.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3186
The math/rand package contains lots of convenient functions, for example
to get an integer in a specified range without running into issues
caused by just truncating a number from a different distribution and so
on. But it's insecure, and we use if for things that benefit from being
more secure like session IDs, CSRF tokens and API keys.
This implements a math/rand.Source that reads from crypto/rand.Reader,
this bridging the gap between them. It also updates our RandomString to
use the new source, thus giving us secure session IDs and CSRF tokens.
Some future work remains:
- Fix API keys by making the generation in the UI use this code as well
- Refactor out these things into an actual random package, and audit
our use of randomness everywhere
I'll leave both of those for the future in order to not muddy the waters
on this diff...
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3180
Switch to my forked version which contains a fix for this issue. I'll
track upstream in the future if things update there, and attempt to
contribute back fixes...
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3149
Without this the summary service doesn't know to recalculate completion
percentage for remote devices when DownloadProgress messages come in.
That means that completion percentage isn't updated in the GUI while
transfers of large files are ongoing. With this change, it updates
correctly.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3144
I think this better reflects what it means. Also tweaks the verbose
format to be more like our other things and lightly refactors the code
to not have the boolean and include the folder in the event.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3121
This was fixed upstream due to our ticket, so we no longer need the
manual handling of commas. Keep the tests and better debug output around
though.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3081
The old usage pattern was to create a Walker with a bunch of attributes,
then call Walk() on it and nothing else. This extracts the attributes
into a Config struct and exposes a Walk(cfg Config) method instead, as
there was no reason to expose the state-holding walker type.
Also creates a few no-op implementations of the necessary interfaces
so that we can skip nil checks and simiplify things here and there.
Definitely look at this diff without whitespace.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3060
Just an optimization. Required exposing the priority from the factory,
so made that an interface with an extra method instead of just a func
type.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3071
This fixes the deadlock by reducing where we hold the various locks. To
start with it splits up the existing "mut" into a "listenersMut" and a
"curConMut" as these are the two things being protected and I can see no
relation between them that requires a shared lock. It also moves all
model calls outside of the lock, as I see no reason to hold the lock
while calling the model (and it's risky, as proven).
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3069
When doing prefix scans in the database, "foo" should not be considered
a prefix of "foo2". Instead, it should match "foo" exactly and also
strings with the prefix "foo/". This is more restrictive than what the
standard leveldb prefix scan does so we add some code to enforce it.
Also exposes the initialScanCompleted on the rwfolder for testing, and
change it to be a channel (so we can wait for it from another
goroutine). Otherwise we can't be sure when the initial scan has
completed, and we need to wait for that or it might pick up changes
we're doing at an unexpected time.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3067
The VersioningConfig change is because it defaults to nil but gets
deserialized to map[string]string{}. Now prepare() enforces a single
representation of the empty map.
GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3065
