-
Notifications
You must be signed in to change notification settings - Fork 74
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
Improve examples of purging #51
Conversation
This makes it a little easier to find which parts are docs and which parts are examples.
Wrap to 80 columns Use hash colons One operation per line
This is great. 👍 |
Let's wait for feedback from @kigster before merging. |
This looks great – except it does not say a word about multi-threading. Is reusing service object in a ruby-process (as a singleton) provides thread-safe API access? We use the gem primarily in highly concurrent background jobs with a thread count up to 40. It would be great if the gem explicitly declare whether or not it is thread safe, especially when used this way. I wish there was a service that would automatically check our code for multi-threaded bugs and give you a badge if the gem passes :) |
I did some quick research and looking around. Some references:
Looking through the code in the gem, it appears that there are several places where key objects are instantiated via module Fetcher # :nodoc: all
# Get the current Fastly::Client
def client(opts = {})
@client ||= Client.new(opts)
end
.... So it's possible to create two clients... and you may end up with multiple instances of Finally the s = Timeout.timeout(@open_timeout, Net::OpenTimeout) {
TCPSocket.open(conn_address, conn_port, @local_host, @local_port)
} I then grepped ruby's standard Net library for 'thread', and to my surprise the only class that seems to care about multi-threading is TL;DR Your desire to reuse these top-level objects at the application startup time is very reasonable. Given the rise of ruby multi-threading, especially through projects like Sidekiq it is important to understand if that type of usage would still behave correctly when called by 50-100 concurrent threads. We are on ruby 2.1.5p273 and we bumped into a all sorts of issues with multi-threading as a bulk of our heavy duty processing is happening in sidekiq, including purging various surrogate keys. Hope this gives a bit more clarity on the issue and why we chose to instantiate each object on each method call, and yes it is a hack :) But the real solution is the verify gem's code and it's dependencies (such as Net::HTTP) for multi-threading, and when finished – it should be very prominent part of the README. K |
Improve examples, especially of purging
We can't guarantee the thread safety of the parts of ruby we use, especially Net::HTTP. In present versions Net::HTTP is not thread-safe. You must create one Net::HTTP object per thread. Future versions may not have that restriction, though. For your case where you want to do many, many purges we recommend that you use the API directly through an HTTP library of your choice. You should be able to send purges more efficiently (fewer operations, fewer objects created) than with fastly-ruby. |
Eric, Everything you said above is true, and would apply to nearly every gem out there. Because of this, we routinely remove gems that are not thread safe from our codebase. However in this particular case, we are dealing with a gem built by a company (Fastly, @crucially) and offered as a client library to talk to Fastly API by Fastly's customers. Therefore I would expect higher standard from this gem, compared to some random gem built by an individual. And people will use your library in multi-threaded environment, as now Sidekiq is being installed on Heroku, and it's now the default background job processing tool there. So multi-threading is here to stay, and is on the rise, after ruby 2.0. I see two possible solutions to this:
TL;DR We used this gem in way that we thought was reasonable, but due to how it is designed, our usage caused a flood of requests to /service hook, which forced Fastly to block us. This is not a great customer experience, and I am a bit surprised that the issue originated from our use of chaining constructors, which is something many other frameworks do, like Rails, etc. Disclaimer, we are customer of Fastly, and personally I am a huge fan of Fastly and what the team accomplished, and I just want the experience of integrating with Fastly's API be smooth for everyone. Because this may happen to someone else in the near future. K |
This addresses #50 through improved documentation.
We didn't have this documented at all, so customers would not be aware of the possible performance impact. We also add pointers to the Fastly API documentation for further reading.