Details

    • Type: Bug Bug
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: JRuby 1.7.0.pre1
    • Component/s: Core Classes/Modules
    • Labels:
      None
    • Environment:
      jruby 1.6.1 (ruby-1.8.7-p330) (2011-05-07 4705f9e) (OpenJDK 64-Bit Server VM 1.6.0_22) [linux-amd64-java]
    • Number of attachments :
      1

      Description

      When CRuby is loading a feature, $LOADED_FEATURES does not include the feature. JRuby includes it.

      % echo 'p $LOADED_FEATURES' > foo.rb
      % ruby -I. -rfoo -e 'load "foo.rb"'
      ["enumerator.so", "/usr/local/lib/ruby/1.9.1/x86_64-linux/enc/encdb.so", "/usr/local/lib/ruby/1.9.1/x86_64-linux/enc/trans/transdb.so"]
      ["enumerator.so", "/usr/local/lib/ruby/1.9.1/x86_64-linux/enc/encdb.so", "/usr/local/lib/ruby/1.9.1/x86_64-linux/enc/trans/transdb.so", "/home/nahi/git/jruby/foo.rb"]
      % ruby187 -I. -rfoo -e 'load "foo.rb"'
      ["enumerator.so"]
      ["enumerator.so", "foo.rb"]
      % jruby -I. -rfoo -e 'load "foo.rb"'  
      ["enumerator.jar", "foo.rb"]
      ["enumerator.jar", "foo.rb"]
      

      Following ad-hoc patch breaks LoadService. You can't build jruby because of generate-ri-cache failure.

      diff --git a/src/org/jruby/runtime/load/LoadService.java b/src/org/jruby/runtime/load/LoadService.java
      index b9eb6b9..a80e809 100644
      --- a/src/org/jruby/runtime/load/LoadService.java
      +++ b/src/org/jruby/runtime/load/LoadService.java
      @@ -746,13 +746,15 @@ public class LoadService {
                   synchronized (loadedFeaturesInternal) {
                       if (featureAlreadyLoaded(loadNameRubyString)) {
                           return false;
      -                } else {
      -                    addLoadedFeature(loadNameRubyString);
                       }
                   }
                   
                   // otherwise load the library we've found
                   state.library.load(runtime, false);
      +
      +            synchronized (loadedFeaturesInternal) {
      +                addLoadedFeature(loadNameRubyString);
      +            }
                   return true;
               } catch (MainExitException mee) {
                   // allow MainExitException to propagate out for exec and friends
      

        Activity

        Hide
        Hiroshi Nakamura added a comment -

        Thanks for the explanation. Now I've understood the intent of your patch!

        Accessing to loadedFeatures directly would cause multi-threaded issue I thought. Your patch depends on the fact that $LOADED_FEATURES is monotonic increase, right?

        I'll find another way...

        For others: the caseInsensitiveFS change is not merged yet (So master must be OK).

        Show
        Hiroshi Nakamura added a comment - Thanks for the explanation. Now I've understood the intent of your patch! Accessing to loadedFeatures directly would cause multi-threaded issue I thought. Your patch depends on the fact that $LOADED_FEATURES is monotonic increase, right? I'll find another way... For others: the caseInsensitiveFS change is not merged yet (So master must be OK).
        Hide
        Hiroshi Nakamura added a comment - - edited

        I checked the CRuby 187 behavior on Windows.

        >copy con Foo.rb
        p :Foo
        ^Z
        
        >ruby187 -rFOO -rFoo -rfoo -e "p $LOADED_FEATURES"
        :Foo
        :Foo
        :Foo
        ["enumerator.so", "FOO.rb", "Foo.rb", "foo.rb"]
        
        >ruby192 -I. -rFOO -e "
        p $LOADED_FEATURES"
        :Foo
        [blah, blah, "C:/Users/nahi/Documents/Foo.rb"]
        
        >jruby156 -I. -rFOO -rFoo -rfoo -e "p $LOADED_FEATURES"
        :Foo
        :Foo
        :Foo
        ["builtin/core_ext/symbol.rb", "enumerator.jar", "FOO.rb", "Foo.rb", "foo.rb"]
        
        >jruby156 -I. --1.9 -rFOO -rFoo -rfoo -e "p $LOADED_FEATURES"
        :Foo
        [blah, blah, "C:/Users/nahi/Documents/Foo.rb"]
        

        So it might be enough to remove whole caseInsensitiveFS condition for compatibility. JRuby should require it 3 times with 1.8 mode, 1 time with 1.9 mode.

        John, can you try the change (remove caseInsensitiveFS block) on your env to measure performance change?

        Show
        Hiroshi Nakamura added a comment - - edited I checked the CRuby 187 behavior on Windows. >copy con Foo.rb p :Foo ^Z >ruby187 -rFOO -rFoo -rfoo -e "p $LOADED_FEATURES" :Foo :Foo :Foo ["enumerator.so", "FOO.rb", "Foo.rb", "foo.rb"] >ruby192 -I. -rFOO -e " p $LOADED_FEATURES" :Foo [blah, blah, "C:/Users/nahi/Documents/Foo.rb"] >jruby156 -I. -rFOO -rFoo -rfoo -e "p $LOADED_FEATURES" :Foo :Foo :Foo ["builtin/core_ext/symbol.rb", "enumerator.jar", "FOO.rb", "Foo.rb", "foo.rb"] >jruby156 -I. --1.9 -rFOO -rFoo -rfoo -e "p $LOADED_FEATURES" :Foo [blah, blah, "C:/Users/nahi/Documents/Foo.rb"] So it might be enough to remove whole caseInsensitiveFS condition for compatibility. JRuby should require it 3 times with 1.8 mode, 1 time with 1.9 mode. John, can you try the change (remove caseInsensitiveFS block) on your env to measure performance change?
        Hide
        John Firebaugh added a comment -

        Certainly removing the caseInsensitiveFS branch entirely will be even better than my change... not sure if it can be done, check JRUBY-5320.

        I'm not sure that the synchronization on loadedFeatures is necessary at all given the other synchronization changes that have been added since then. Note that iterating over a concurrent collection using an Iterator provides only weak concurrency guarantees – it's probably the same with a `for (Object str : loadedFeaturesInternal)` loop. So my guess is that if synchronization is necessary there, it needs to be an explicit `synchronized(loadedFeatures)` anyway. But as I said, I'm skeptical that it is necessary.

        Show
        John Firebaugh added a comment - Certainly removing the caseInsensitiveFS branch entirely will be even better than my change... not sure if it can be done, check JRUBY-5320 . I'm not sure that the synchronization on loadedFeatures is necessary at all given the other synchronization changes that have been added since then. Note that iterating over a concurrent collection using an Iterator provides only weak concurrency guarantees – it's probably the same with a `for (Object str : loadedFeaturesInternal)` loop. So my guess is that if synchronization is necessary there, it needs to be an explicit `synchronized(loadedFeatures)` anyway. But as I said, I'm skeptical that it is necessary.
        Hide
        Hiroshi Nakamura added a comment -

        Thanks for confirmation. For case-sensitivity, I think JRUBY-5320 and http://redmine.ruby-lang.org/issues/show/4255 both should be reverted. $LOADED_FEATURES includes Strings given from FS so it should not be an issue even if case-insensible FS. I'm asking this to the assigned CRuby commiter. I'll write again.

        For synchronization, using loadedFeatureInternal should be safe. But as you said simple synchronization is better if we should iterate it. Synchronization is needed because we have public LoadService#removeInternalLoadedFeature() which does not share a lock for require. I hope we can drop removeInternalLoadedFeature()...

        Show
        Hiroshi Nakamura added a comment - Thanks for confirmation. For case-sensitivity, I think JRUBY-5320 and http://redmine.ruby-lang.org/issues/show/4255 both should be reverted. $LOADED_FEATURES includes Strings given from FS so it should not be an issue even if case-insensible FS. I'm asking this to the assigned CRuby commiter. I'll write again. For synchronization, using loadedFeatureInternal should be safe. But as you said simple synchronization is better if we should iterate it. Synchronization is needed because we have public LoadService#removeInternalLoadedFeature() which does not share a lock for require. I hope we can drop removeInternalLoadedFeature()...
        Hide
        Hiroshi Nakamura added a comment -

        I merged require_protection branch to master at d54bb64ee61fb46b3ed2836f76c25dc9d9ce1a2d.
        That branch also included autoload thread issue(JRUBY-3194), too and I think it's a little big change for maintenance branch.

        I close this ticket as 'Fixed in 1.7' but feel free to reopen it if you need this fix for jruby-1_6 as well.

        Show
        Hiroshi Nakamura added a comment - I merged require_protection branch to master at d54bb64ee61fb46b3ed2836f76c25dc9d9ce1a2d. That branch also included autoload thread issue( JRUBY-3194 ), too and I think it's a little big change for maintenance branch. I close this ticket as 'Fixed in 1.7' but feel free to reopen it if you need this fix for jruby-1_6 as well.

          People

          • Assignee:
            Hiroshi Nakamura
            Reporter:
            Hiroshi Nakamura
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: