Threading fixes #1142

wants to merge 3 commits into


None yet

2 participants

Smarre commented Oct 21, 2013


First commit about list() should be ok as it is, atleast as far as I know.

The second one, I don’t really know the cause, or how this really affects, but it did get me a bit further. I’m having some strange threading related problems, which mostly are just due activerecord being not really multithreading compatible, but some strange NPEs and other errors too. So I decided to include this fix too.

Smarre added some commits Oct 21, 2013
@Smarre Smarre RubyThreadGroup: make list() thread-safe.
If two different threads accesses list() at same time, `nextEntry': java.util.ConcurrentModificationException
@Smarre Smarre Add a guard against nil RubyThreadGroup for a thread.
This is just here since I seem to get this error quite a bit.
I guess correct way would be to ensure somehow that there always
is the default group, but quick search didn’t give me any idea
how it can be missing in first place.
@Smarre Smarre make callEventHooks() thread-safe
It seems that concurrent calls to this method causes
Exception in thread "RubyThread-3: .." java.util.ConcurrentModificationException.
Smarre commented Oct 21, 2013

I added another synchronized function commit.

headius commented Oct 21, 2013

These are good fixes but I think I know some better ways to solve them without hard synchronization everywhere. I'll credit you for the finds, though.

@headius headius added a commit that referenced this pull request Oct 21, 2013
@headius headius Use COW list for event hooks, to avoid synchronization.
See jruby/jruby#1142. This supercedes a fix there.

I opted to use CopyOnWriteArrayList here for two reasons:

* It does not require synchronization, neither for access nor iter
* EventHooks should are only added or removed rarely
headius commented Oct 21, 2013

I used your fix for RubyThreadGroup#list and the nil RubyThreadGroup issue and a different approach for the EventHooks list. Thank you!

@headius headius closed this Oct 21, 2013
Smarre commented Oct 21, 2013

No problems, better have better fixes :) Thanks for merging.

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