Skip to content

Conversation

@reidmix
Copy link
Contributor

@reidmix reidmix commented Feb 6, 2015

When using multiple threads we are running into a scenario using SideKiq/SideTiq where the mutex in Scrolls::Log#write isn't doing what we expect. Even with the mutex, it is not shared across threads, and in the following example, the two loglines are not being separated out with a newline as we'd expect:

2014-09-18T04:57:00.897495+00:00 54.80.127.117 local7.info app[worker.1]: - d.fdd8e8d0-048c-4667-9199-218114219753 sample#app.q.enqueued=0 sample#app.q.processed=1 sample#watchtower.q.failed=0 measure#app.q.latency=02014-09-18T04:57:00.897Z 2 TID-owg6kvhpo INFO: [Sidetiq] Enqueue: Stats (at: 1411016280.0) (last: 1411016220.0)

To simplify the case, we can see that puts is not atomic:

1.9.3-p448 :015 >   10.times { Thread.new { puts "hi" }}
 => 10 
1.9.3-p448 :016 > hihihihi
hi

hihi


hi
hihi

A solution is to use print, it is atomic and works as expected (even without the mutex):

1.9.3-p448 :016 > 10.times { Thread.new { print "hi\n" }}
hi
hi
hi
 => 10 
hi
hi
hi
hi
hi
hi

Additionally, removing the rescue that was an artifact from an old refactor:
60e7220

@reidmix
Copy link
Contributor Author

reidmix commented Feb 6, 2015

Tagging: @dickeyxxx who worked on this fix with me, @pedro who determined the solution to this bug, and @amerine who reported this issue

@jdx
Copy link

jdx commented Feb 6, 2015

In a situation using multiple processes or a setup like sidekiq that wouldn't share the mutex, you could see this come up. It would generally be very rare since it would be 2 different threads/processes running puts at the same time.

I think we might be seeing it since we're running 2 schedulers (which is an issue by itself, I'll admit), but performing the log atomically vs a mutex I think is a better solution to the problem anyways.

@eric
Copy link

eric commented Feb 8, 2015

👍

@amacneil
Copy link

👍

We have also been seeing this for months (see #64)

@mikehale
Copy link
Contributor

👍

@asenchi
Copy link
Owner

asenchi commented Mar 20, 2015

Not sure why I never received any of these emails.

Regardless this seems pretty straight forward.

asenchi added a commit that referenced this pull request Mar 20, 2015
print is atomic instead of mutex so it works in threaded environs
@asenchi asenchi merged commit b4a7619 into asenchi:master Mar 20, 2015
@jmervine
Copy link

jmervine commented Feb 4, 2016

Mind updating @reidmix and I when when this is released?

rwz added a commit to heroku/l2meter that referenced this pull request Apr 25, 2016
@dbussink
Copy link
Contributor

Looks like this broke using syslog, since the syslog class only has a puts method: https://github.com/asenchi/scrolls/blob/master/lib/scrolls/syslog.rb#L38.

@asenchi
Copy link
Owner

asenchi commented Aug 18, 2017

Hello @dbussink Good call out. Looking at your PR as well. I'm picking things back up and hope to get the project cleaned up over the next week. Sorry for the lack of maintenance here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants