Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add better support for GlobalId and active job #150

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

### 0.9.2 (Next)

* [#150](https://github.com/codegram/hyperclient/pull/150): Add better support for globalid and active job - [@yuki24](https://github.com/yuki24).
* Your contribution here.

### 0.9.1 (2019/08/25)
Expand Down
2 changes: 2 additions & 0 deletions hyperclient.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,6 @@ Gem::Specification.new do |gem|
gem.add_dependency 'faraday_hal_middleware'
gem.add_dependency 'faraday_middleware'
gem.add_dependency 'net-http-digest_auth'

gem.add_development_dependency 'globalid'
end
9 changes: 8 additions & 1 deletion lib/hyperclient.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,23 @@
require 'hyperclient/resource'
require 'hyperclient/resource_collection'
require 'hyperclient/version'
require 'hyperclient/railtie' if defined?(Rails)

# Public: Hyperclient namespace.
#
module Hyperclient
URL_TO_ENDPOINT_MAPPING = { }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a constant, shouldn't be CAPS.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reference seems constant, as it's the object itself that will be mutated, right?


# Public: Convenience method to create new EntryPoints.
#
# url - A String with the url of the API.
#
# Returns a Hyperclient::EntryPoint
def self.new(url, &block)
Hyperclient::EntryPoint.new(url, &block)
URL_TO_ENDPOINT_MAPPING[url] = Hyperclient::EntryPoint.new(url, &block)
Copy link
Contributor

@ivoanjo ivoanjo Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello there! I may be missing a bit of context on how this works, but I have a few notes:

  • This creates a memory leak -- every time we create a new Hyperclient instance we add it to this map, and never remove it. Have you considered changing this so it won't grow forever?

  • This is problematic if you're running with a Ruby with support for parallelism, as many threads can be competing to write to the hash. Since everyone's just writing the intuition may be that there's no problem, and indeed on MRI Ruby since a Hash is implemented in C this has no problem, but on other Rubies this can be seen. Here's my quick experiment:

puts RUBY_DESCRIPTION

PER_THREAD_LIMIT = 1000000
THREADS = 8

THE_HASH = {}

(1..THREADS).to_a.map do |thread_id|
  Thread.new do
    PER_THREAD_LIMIT.times do
      THE_HASH[rand(PER_THREAD_LIMIT)] = thread_id
    end
    puts "Thread #{thread_id} done!"
  end
end.map(&:join)

puts "All done!"

Running this on TruffleRuby:

truffleruby 19.1.1, like ruby 2.6.2, GraalVM CE Native [x86_64-linux]
Thread 1 done!
threads.rb:11:in `[]=': <no message> (NullPointerException) (RuntimeError)
        from org.truffleruby.core.hash.PackedArrayStrategy.getHashed(PackedArrayStrategy.java:41)
        from org.truffleruby.core.hash.SetNode.setPackedArray(SetNode.java:80)
        from org.truffleruby.core.hash.SetNodeGen.executeSet(SetNodeGen.java:37)
        from org.truffleruby.core.hash.HashNodes$SetIndexNode.set(HashNodes.java:239)
        from org.truffleruby.core.hash.HashNodesFactory$SetIndexNodeFactory$SetIndexNodeGen.execute(HashNodesFactory.java:632)
        from org.truffleruby.language.control.SequenceNode.execute(SequenceNode.java:34)
        from org.truffleruby.language.methods.ExceptionTranslatingNode.execute(ExceptionTranslatingNode.java:51)
        from org.truffleruby.language.RubyRootNode.execute(RubyRootNode.java:54)
        from org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.callProxy(OptimizedCallTarget.java:328)
        from org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.callRoot(OptimizedCallTarget.java:318)
Translated to internal error
        from threads.rb:11:in `block (3 levels) in <main>'
        from threads.rb:10:in `times'
        from threads.rb:10:in `block (2 levels) in <main>'

Running this on JRuby:

jruby 9.2.8.0 (2.5.3) 2019-08-12 a1ac7ff OpenJDK 64-Bit Server VM 11.0.3+7-LTS on 11.0.3+7-LTS +jit [linux-x86_64]
warning: thread "Ruby-0-Thread-3: threads.rb:1" terminated with exception (report_on_exception is true):
java.lang.ArrayIndexOutOfBoundsException: Index 18581 out of bounds for length 16427
        at org.jruby.dist/org.jruby.RubyHash.internalPutNoResize(RubyHash.java:561)
        at org.jruby.dist/org.jruby.RubyHash.internalPut(RubyHash.java:535)
        at org.jruby.dist/org.jruby.RubyHash.internalPut(RubyHash.java:525)
        at org.jruby.dist/org.jruby.RubyHash.fastASetCheckString(RubyHash.java:1020)
        at org.jruby.dist/org.jruby.RubyHash.op_aset(RubyHash.java:1055)
        at org.jruby.dist/org.jruby.RubyHash$INVOKER$i$2$0$op_aset.call(RubyHash$INVOKER$i$2$0$op_aset.gen)
        at org.jruby.dist/org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:203)
        at threads.invokeOther1:\=\{\}=(threads.rb:11)
        at threads.RUBY$block$\=threads\,rb$2(threads.rb:11)
        at org.jruby.dist/org.jruby.runtime.CompiledIRBlockBody.yieldDirect(CompiledIRBlockBody.java:146)
        at org.jruby.dist/org.jruby.runtime.IRBlockBody.yieldSpecific(IRBlockBody.java:85)
        at org.jruby.dist/org.jruby.runtime.Block.yieldSpecific(Block.java:139)
        at org.jruby.dist/org.jruby.RubyFixnum.times(RubyFixnum.java:279)
        at org.jruby.dist/org.jruby.RubyInteger$INVOKER$i$0$0$times.call(RubyInteger$INVOKER$i$0$0$times.gen)
        at org.jruby.dist/org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:151)
        at org.jruby.dist/org.jruby.runtime.callsite.CachingCallSite.callIter(CachingCallSite.java:160)
        at threads.invokeOther3:times(threads.rb:10)
        at threads.RUBY$block$\=threads\,rb$1(threads.rb:10)
        at org.jruby.dist/org.jruby.runtime.CompiledIRBlockBody.callDirect(CompiledIRBlockBody.java:136)
        at org.jruby.dist/org.jruby.runtime.IRBlockBody.call(IRBlockBody.java:77)
        at org.jruby.dist/org.jruby.runtime.Block.call(Block.java:129)
        at org.jruby.dist/org.jruby.RubyProc.call(RubyProc.java:295)
        at org.jruby.dist/org.jruby.RubyProc.call(RubyProc.java:274)
        at org.jruby.dist/org.jruby.RubyProc.call(RubyProc.java:270)
        at org.jruby.dist/org.jruby.internal.runtime.RubyRunnable.run(RubyRunnable.java:105)
        at java.base/java.lang.Thread.run(Thread.java:834)

Thread 5 done!
Thread 8 done!
Thread 4 done!
Thread 6 done!
Thread 2 done!
Thread 1 done!
Unhandled Java exception: java.lang.ArrayIndexOutOfBoundsException: Index 18581 out of bounds for length 16427
java.lang.ArrayIndexOutOfBoundsException: Index 18581 out of bounds for length 16427
   internalPutNoResize at org/jruby/RubyHash.java:561
           internalPut at org/jruby/RubyHash.java:535
           internalPut at org/jruby/RubyHash.java:525
   fastASetCheckString at org/jruby/RubyHash.java:1020
               op_aset at org/jruby/RubyHash.java:1055
                  call at org/jruby/RubyHash$INVOKER$i$2$0$op_aset.gen:-1
                  call at org/jruby/runtime/callsite/CachingCallSite.java:203
  invokeOther1:\=\{\}= at threads.rb:11
            threads.rb at threads.rb:11
           yieldDirect at org/jruby/runtime/CompiledIRBlockBody.java:146
         yieldSpecific at org/jruby/runtime/IRBlockBody.java:85
         yieldSpecific at org/jruby/runtime/Block.java:139
                 times at org/jruby/RubyFixnum.java:279
                  call at org/jruby/RubyInteger$INVOKER$i$0$0$times.gen:-1
                  call at org/jruby/runtime/callsite/CachingCallSite.java:151
              callIter at org/jruby/runtime/callsite/CachingCallSite.java:160
    invokeOther3:times at threads.rb:10
            threads.rb at threads.rb:10
            callDirect at org/jruby/runtime/CompiledIRBlockBody.java:136
                  call at org/jruby/runtime/IRBlockBody.java:77
                  call at org/jruby/runtime/Block.java:129
                  call at org/jruby/RubyProc.java:295
                  call at org/jruby/RubyProc.java:274
                  call at org/jruby/RubyProc.java:270
                   run at org/jruby/internal/runtime/RubyRunnable.java:105
                   run at java/lang/Thread.java:834

This won't happen often, of course -- I needed to create a few threads and a lot of items, but it's the kind of thing that can bite you at the worst possible time, e.g. when doing a deployment with high-traffic with a threaded webserver.

My suggestion for this one would be to consider using a Concurrent::Map from the concurrent-ruby gem, or using a lock to protect the map while writing.

end

def self.lookup_entry_point(uri)
URL_TO_ENDPOINT_MAPPING[uri] || raise(ArgumentError, "Entry point not registered for #{uri}")
end
end
56 changes: 56 additions & 0 deletions lib/hyperclient/global_id.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# frozen_string_literal: true

require 'delegate'
require 'globalid'

module Hyperclient
module GlobalId
class Locator
def locate(gid)
Hyperclient
.lookup_entry_point(gid.params['endpoint'])
.public_send(gid.params['key'], gid.params.except('app', 'endpoint', 'key'))
end
end

class Serializable < SimpleDelegator
def id
_url
end
end

private_constant :Serializable

def self.app_name
"#{GlobalID.app}-hyperclient"
end

def self.setup!
::Hyperclient::Link.include ::GlobalID::Identification
::Hyperclient::Link.prepend ::Hyperclient::GlobalId

::GlobalID::Locator.use app_name, ::Hyperclient::GlobalId::Locator.new
end

def to_global_id(options = {})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: I suggest using the more modern **options e.g. to_global_id(**options), as it also ensures that all keys are symbols, thus avoiding mistakes when using strings instead.

GlobalID.create(Serializable.new(self), default_global_id_options.merge(options))
end
alias to_gid to_global_id

def to_signed_global_id(options = {})
SignedGlobalID.create(Serializable.new(self), default_global_id_options.merge(options))
end
alias to_sgid to_signed_global_id

private

def default_global_id_options
{
app: ::Hyperclient::GlobalId.app_name,
endpoint: @entry_point._url,
key: @key,
**@uri_variables || {},
}
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Don't forget to configure your editor to add a newline at the end of the file :)

11 changes: 11 additions & 0 deletions lib/hyperclient/railtie.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
module Hyperclient
class Railtie < ::Rails::Railtie #:nodoc:
initializer 'hyperclient.client.attach_log_subscriber' do
ActiveSupport.on_load(:active_job) do
require 'hyperclient/global_id'

::Hyperclient::GlobalId.setup!
end
end
end
end
31 changes: 30 additions & 1 deletion test/hyperclient/link_test.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require_relative '../test_helper'
require 'hyperclient'
require 'hyperclient/global_id'

module Hyperclient
describe Link do
Expand Down Expand Up @@ -350,5 +351,33 @@ module Hyperclient
end
end
end

describe 'GlobalId Support' do
describe '#to_global_id' do
before do
Hyperclient::GlobalId.setup!
Hyperclient::URL_TO_ENDPOINT_MAPPING[entry_point._url] = entry_point
end

it "serializes the object with entry point, key and uri_variables if present" do
link = Link.new('key', { 'href' => "http://api.example.org/key" }, entry_point)

stub_request(entry_point.connection) do |stub|
stub.get('http://api.example.org/') { [200, {}, { '_links' => { 'key' => { 'href' => 'http://api.example.org/key' } } }] }
end

actual = GlobalID.find(link.to_global_id)

actual._url.must_equal(link._url)
end

it "raises an exception when the hypermedia URL is missing" do
link_without_href = Link.new('key', { }, entry_point)
message = "Unable to create a Global ID for Hyperclient::GlobalId::Serializable without a model id."

-> { link_without_href.to_global_id }.must_raise(URI::GID::MissingModelIdError, message)
end
end
end
end
end
end