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.

Sign up for GitHub

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

Jump to bottom

Add better support for GlobalId and active job #150

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add better support for GlobalId and active job #150

wants to merge 1 commit into from

Conversation

yuki24
Copy link
Contributor

@yuki24 yuki24 commented Aug 25, 2019

This PR adds better support for ActiveJob and GlobalId and will make it dramatically. This is still a WIP, but Artsy will start using this branch shortly.

Before

yuki = Gravity.client.user_detail(id: "yuki-id")
UsersMailer.reset_password(user_detail: yuki, reset_password_token: 'xyz').deliver_later
# => ActiveJob::SerializationError: Unsupported argument type: Hyperclient::Link

After

yuki = Gravity.client.user_detail(id: "yuki-id")
UsersMailer.reset_password(user_detail: yuki, reset_password_token: 'xyz').deliver_later
# => Enqueued ActionMailer::DeliveryJob (Job ID: ...)

Todo

  • Add tests for the #to_global_id method
  • Add tests for the #to_signed_global_id method
  • Implement the same interface in Hyperclient::Link
  • Implement the same interface in Hyperclient::LinkCollection
  • Implement the same interface in Hyperclient::Resource
  • Implement the same interface in Hyperclient::ResourceCollection

@yuki24 yuki24 force-pushed the globalid-and-active-job branch 2 times, most recently from 9e09bbb to fc9af92 Compare August 25, 2019 15:45
dblock
dblock reviewed Aug 25, 2019
View reviewed changes
Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I worry that we're bringing in the kitchensink of Rails. Would it make more sense to make a separate library hyperclient-globalid or hyperclient-rails instead

lib/hyperclient.rb

# 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?

@yuki24
Copy link
Contributor Author

yuki24 commented Aug 25, 2019

Hmm I understand the concern but smaller gems are just a burden to maintain. But I'm not strongly against that idea.

ivoanjo
ivoanjo suggested changes Aug 26, 2019
View reviewed changes
Copy link
Contributor

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

Left a few notes!

Since no new dependency is needed to support this, may I suggest just keeping all of the changes in a separate folder (which would be basically the folder that would be moved over to a hypothetical hyperclient-rails), and only adding the conditional load on hyperclient.rb?

That way I think we can get the best of both worlds: we keep the rails changes clearly separated, but we don't need the extra dependency and since we test both hyperclient and the rails changes together, we also get the advantage that we don't accidentally break it with some other future change.

lib/hyperclient/global_id.rb
}
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 :)

lib/hyperclient/global_id.rb
::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.

lib/hyperclient.rb
# 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 •
edited
Loading

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.

lib/hyperclient.rb

# Public: Hyperclient namespace.
#
module Hyperclient
URL_TO_ENDPOINT_MAPPING = { }
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?

@dblock
Copy link
Collaborator

dblock commented Aug 26, 2019

Hmm I understand the concern but smaller gems are just a burden to maintain. But I'm not strongly against that idea.

I think in the case of support for Rails it makes more sense to split. It's a common pattern. Rails keeps adding kitchen-sink type things and this makes things harder to maintain. That said I don't have that strong of feelings about it, I just thought I'd bring it up.

@ivoanjo
Copy link
Contributor

ivoanjo commented Sep 10, 2019

Hey @yuki24 I hope I didn't scare you with my comments, I'm definitely willing to help out with solving those :)

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

@dblock dblock dblock left review comments

@ivoanjo ivoanjo ivoanjo requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yuki24 @dblock @ivoanjo