Address FileThreadPool race condition

[wfleming]

I saw a couple ThreadErrors around popping from an empty queue in
bugsnag: this got introduced when I moved away from catching all
ThreadErrors. There's a race condition between checking that the queue
is empty & attempting to pop from it.

This change locks the pop operation with a mutex to prevent the race.

👀 @codeclimate/review

[pbrisbin]

WDYT of a small utility method:

# yield item while (item = next_item(queue))

def next_item(queue)
  lock = Mutex.new
  lock.synchronize { !queue.empty? && queue.pop(true) }
end

Another option would be rescue the non-block-empty-queue-popped exception (assuming it's sufficiently specific), and replace it with a nil.

[wfleming]

WDYT of a small utility method:

That sounds good to me, but I think the lock would need to be a member var, yeah? Getting a new lock on every method call doesn't seem real helpful.

Another option would be rescue the non-block-empty-queue-popped exception (assuming it's sufficiently specific)

It's a ThreadError, and I felt that was too broad to rescue: previously we were rescuing them in the entire thread block. I could wrap just the pop with a rescue, but I'm not confident enough about JRuby threading internals to be sure nothing else could raise that exception class at any given time.

[pbrisbin]

I think the lock would need to be a member var, yeah? Getting a new lock on every method call doesn't seem real helpful.

Can you expand on that for my own education? I'm not familiar with this and assumed it didn't matter from where you instantiated the Mutex, it would always work the same.

[pbrisbin]

Oh I see. It's not a global lock, you have to use the same instance in both (all) threads for it to actually prevent the concurrency. Sorry about that.

Instance var seems fine. The FP purist in me would probably pass it into #next_item, but this is rubby so, eh.

[wfleming]

@pbrisbin Thanks for that, updated.

(FWIW the single line version of the loop failed to parse: I think it's because yield is special.)

[pbrisbin]

I think it's just the paren-less method call. yield(item) while ... would've probably worked (among other incantations).

Anyway, I like the multi-line way. LGTM.