JRuby

Array#pack is no longer threadsafe

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: JRuby 1.6.1
  • Fix Version/s: JRuby 1.6.2
  • Component/s: Core Classes/Modules
  • Labels:
    None
  • Environment:
    independent - observed on windows and linux
  • Testcase included:
    yes
  • Number of attachments :
    3

Description

The method Array#pack is no longer thread-safe. That is, if called on different arrays in different threads it may not encode the array properly. In particular, it may encode one array using the converter for another call.

I have attached a simple test program, which starts two threads each in a tight loop calling pack and checking the result. But, they call pack with different arguments. It will print each time it gets the wrong result. (NOTE: the program will not terminate until you stop it w/CTRL-C or equivalent.)

Looking at the code in org.jruby.util.Pack.java, the issue arises here (line 1830 in jruby 1.6.0):
Converter converter = converters[type];

if (converter != null)

{ executor.setConverter(converter); idx = encode(runtime, occurrences, result, list, idx, executor); continue; }

the variable executor is really executor18, a static variable in this class. Hence, it is shared across all calls to pack. But, this shared static variable has state, in particular from executor.setConverter().

  1. File
    jruby-bug.rb
    20/Apr/11 10:59 AM
    0.5 kB
    rich liebling
  2. Text File
    jruby-pack.patch
    20/Apr/11 4:54 PM
    3 kB
    rich liebling
  3. Text File
    threadsafe-pack.patch
    20/Apr/11 4:54 PM
    3 kB
    rich liebling

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
rich liebling added a comment -

I am attaching two potential patches to resolve this.
jruby-pack.patch

    • To avoid the threading problem, it creates a new executor (executor18 or executor19) object for each call. Very little change to the source, but incurs the cost of an additional object allocation for each invocation of pack().

threadsafe-pack.patch:

    • The only point of the ConverterExecutor is to know whether to call the 1.8 or 1.9 version on the converter. While one option would be to eliminate this class and just query the runtime object to determine which method to invoke on the converter, this patch instead just says go ahead and pass an object that knows which method to invoke, but also pass the converter itself (instead of asking the ConverterExecutor to remember it in a non-threadsafe way).

Apologies for not having written proper unit tests for this.

Show
rich liebling added a comment - I am attaching two potential patches to resolve this. jruby-pack.patch To avoid the threading problem, it creates a new executor (executor18 or executor19) object for each call. Very little change to the source, but incurs the cost of an additional object allocation for each invocation of pack(). threadsafe-pack.patch: The only point of the ConverterExecutor is to know whether to call the 1.8 or 1.9 version on the converter. While one option would be to eliminate this class and just query the runtime object to determine which method to invoke on the converter, this patch instead just says go ahead and pass an object that knows which method to invoke, but also pass the converter itself (instead of asking the ConverterExecutor to remember it in a non-threadsafe way). Apologies for not having written proper unit tests for this.
Hide
Permalink
Charles Oliver Nutter added a comment -

Your first patch is the right way to go, I think. The object-creation overhead should be negligible compared to the parsing and String construction.

I'm amazed we didn't notice this earlier. Thank you!

Master:

commit 1efe46804bf9e80f16b6aaf8059ae574b6856091
Author: Charles Oliver Nutter <headius@headius.com>
Date:   Wed Apr 27 16:46:34 2011 -0500

    Fix JRUBY-5728: Array#pack is no longer threadsafe
    
    Patch by Rich Liebling.

1.6:

commit 6253a9a0d7b2473cc7f752447284551ce2a8387d
Author: Charles Oliver Nutter <headius@headius.com>
Date:   Wed Apr 27 16:46:34 2011 -0500

    Fix JRUBY-5728: Array#pack is no longer threadsafe
    
    Patch by Rich Liebling.
Show
Charles Oliver Nutter added a comment - Your first patch is the right way to go, I think. The object-creation overhead should be negligible compared to the parsing and String construction. I'm amazed we didn't notice this earlier. Thank you! Master: commit 1efe46804bf9e80f16b6aaf8059ae574b6856091 Author: Charles Oliver Nutter <headius@headius.com> Date: Wed Apr 27 16:46:34 2011 -0500 Fix JRUBY-5728: Array#pack is no longer threadsafe Patch by Rich Liebling. 1.6: commit 6253a9a0d7b2473cc7f752447284551ce2a8387d Author: Charles Oliver Nutter <headius@headius.com> Date: Wed Apr 27 16:46:34 2011 -0500 Fix JRUBY-5728: Array#pack is no longer threadsafe Patch by Rich Liebling.

People

  • Assignee:
    Charles Oliver Nutter
    Reporter:
    rich liebling
Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: