papertrail / remote_syslog2

To install, see "Releases" tab. Self-contained daemon for reading local files and emitting remote syslog (without using local syslogd).
http://help.papertrailapp.com/
MIT License
637 stars 156 forks source link

Fix race condition leading to duplicate tailing #181

Closed Bowbaq closed 7 years ago

Bowbaq commented 7 years ago

Expected

If a file is matched by two (or more) globs, remote_syslog2 matches the first glob in the list, and ignores subsequent matches

Actual

Sometimes, due to a race condition, a log file ends up being tailed twice. I encountered this today, we're using 0.18. Here's a trace. Notice that the some files are forwarded twice (line 7 and 29 for example).

Fix

Adding the file to the registry synchronously seems to fix the problem. I've added a test (not necessarily a fan of hijacking the logs but ...). Running that test on the current master fails most of the time.

snorecone commented 7 years ago

Hi @Bowbaq, great catch.

About the test that is hijacking logs, would you consider instead making the *WorkerRegistry an interface instead (that responds to Add, Exists, etc., and using a testRegistry{} to count how many times a files is added? It seems like it would be less brittle that way.

Bowbaq commented 7 years ago

@snorecone converted the tests. The build was still failing due to a race condition, but it's a different one. This one is because this read is not protected and the value can be written to concurrently. The last commit should fix

snorecone commented 7 years ago

@Bowbaq thanks, this is great.

Sorry about the race in the test, I'll probably want to revisit the shutdown code later so that mutexes aren't used for every write.

Bowbaq commented 7 years ago

re: mutex I haven't benched it but my guess is, in the absence of contention, acquiring a read lock should be very cheap

snorecone commented 7 years ago

Yeah, I agree. No need to dig into it unless there's a benchmark.

Bowbaq commented 7 years ago

Do you know when the next release will be? We're being billed up double for our logs until we deploy this

snorecone commented 7 years ago

@Bowbaq https://github.com/papertrail/remote_syslog2/releases/tag/v0.19-beta2

Bowbaq commented 7 years ago

Sweet, thanks