Introduce EmptyfileResource that should replace the JRubyNotFoundFile.NOT_FOUND #1513

Merged
merged 2 commits into from Mar 4, 2014

Conversation

Projects
None yet
3 participants
Contributor

ratnikov commented Feb 19, 2014

This pull request also includes a commit that forces to provide Ruby reference when creating any file resources. EmptyFileResource requires a Ruby reference to properly raise errors when trying to access information about a resource that doesn't exist, such as time modified and stat information.

Contributor

ratnikov commented Feb 19, 2014

So I just realized that I pulled against master, although I'd like this to go into 1.7 branch targeting 1.7.12 release. But I guess since there's no special 1.7.11 branch yet, maybe going into master and then pulling into 1.7 makes sense.

Owner

enebo commented Feb 20, 2014

I think pushing the CWD from the top of [] and glob is less prone to multi-threaded weirdness scenarios so it seems like we should be capturing CWD at beginning of these methods and propagating it like we are. I tried to mention this on IRC but we a netsplit happened :)

Contributor

ratnikov commented Feb 20, 2014

Good point about threadness. However, if we're worried about multi-threadness, I think it's better to fix that properly, rather than relying that implementation gets the state early. That would require some kind of read monitor that would be checked out when doing RubyFile/RubyDir stuff, and then released once it's done. Ruby.setCurrentDirectrory would then block until there are no outstanding readers. This might be tricky to implement and is a recipe for deadlocks (code getting read lock and then trying to set current directory, for example). On other hand, changing current directory is a pretty significant operation and should be done with care, so shouldn't happen too often (and maybe we can restrict its usage by visibility).

Contributor

ratnikov commented Feb 20, 2014

By the way, seems like your concern about thread safety is correct:

require 'thread'

Dir.chdir 'core'
Thread.abort_on_exception = true

t1 = Thread.new do
  dirs = %w(core spec test)
  loop do
    # puts "changing to #{dirs.first}\n"
    Dir.chdir File.join('..', dirs.first)
    dirs.rotate!
  end
end


t2 = Thread.new do
  patterns = {}
  loop do
    glob = Dir["**/*"]
    unless patterns.has_key?(glob)
      puts "Newly seen: #{(glob.first(5) + glob.last(5)).inspect}"
      patterns[glob] = true
    else
      puts "same..."
    end
  end
end

sleep 60

Both MRI and current jruby 1.7 branch settle down on "same..." after 3 different globs, whereas my patch behaves erroneously.

Owner

enebo commented Feb 20, 2014

yeah I think the capture CWD at beginning is the right way because it eliminates needing to even think about concurrent access of CWD. I promise you that with your patch we will get future bug reports and propagation is the simplest mechanism for preventing this problem.

Contributor

ratnikov commented Feb 21, 2014

I've updated it to use ThreadContext everywhere, as well as cache CWD. My thread test seems to pass.

PTAL

Owner

headius commented Feb 26, 2014

So what's the status here...more work coming or should we merge?

Contributor

ratnikov commented Feb 26, 2014

Should merge. :)

Contributor

ratnikov commented Mar 3, 2014

After some thinking, it may be possible to avoid having to pass context to file resources and raise a special checked exception that will eventually be converted to regular RaiseException in code that has direct access to context. I'll try to update this PR to reflect that approach, so don't merge just yet. :)

@enebo enebo added a commit that referenced this pull request Mar 4, 2014

@enebo enebo Merge pull request #1513 from ratnikov/null-file-resource
Introduce EmptyfileResource that should replace the JRubyNotFoundFile.NOT_FOUND
2fca72c

@enebo enebo merged commit 2fca72c into jruby:master Mar 4, 2014

enebo added this to the JRuby 9000 milestone Apr 15, 2014

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