-
Notifications
You must be signed in to change notification settings - Fork 85
Proposed SysV structures and Mixin functionality #64
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
Changes from 3 commits
ace7741
bcb187d
144e8ea
9d117e2
89075a2
8c6f6dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -124,7 +124,8 @@ def register(name, tickets:, permissions: 0660, timeout: 0, error_threshold:, er | |
| error_threshold: error_threshold, | ||
| error_timeout: error_timeout, | ||
| exceptions: Array(exceptions) + [::Semian::BaseError], | ||
| implementation: ::Semian::Simple, | ||
| permissions: permissions, | ||
| implementation: ::Semian::SysV, | ||
| ) | ||
| resource = Resource.new(name, tickets: tickets, permissions: permissions, timeout: timeout) | ||
| resources[name] = ProtectedResource.new(resource, circuit_breaker) | ||
|
|
@@ -159,6 +160,10 @@ def resources | |
| require 'semian/simple_sliding_window' | ||
| require 'semian/simple_integer' | ||
| require 'semian/simple_state' | ||
| require 'semian/sysv_shared_memory' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should these
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree. |
||
| require 'semian/sysv_sliding_window' | ||
| require 'semian/sysv_integer' | ||
| require 'semian/sysv_state' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should put these in the block below. |
||
| if Semian.semaphores_enabled? | ||
| require 'semian/semian' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh? Only require |
||
| else | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,16 +2,20 @@ module Semian | |
| class CircuitBreaker #:nodoc: | ||
| extend Forwardable | ||
|
|
||
| def initialize(name, exceptions:, success_threshold:, error_threshold:, error_timeout:, implementation:) | ||
| def initialize(name, exceptions:, success_threshold:, error_threshold:, error_timeout:, permissions:, implementation:) | ||
| @name = name.to_sym | ||
| @success_count_threshold = success_threshold | ||
| @error_count_threshold = error_threshold | ||
| @error_timeout = error_timeout | ||
| @exceptions = exceptions | ||
|
|
||
| @errors = implementation::SlidingWindow.new(max_size: @error_count_threshold) | ||
| @successes = implementation::Integer.new | ||
| @state = implementation::State.new | ||
| @errors = implementation::SlidingWindow.new(max_size: @error_count_threshold, | ||
| name: "#{name}_sysv_sliding_window", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think these names should have |
||
| permissions: permissions) | ||
| @successes = implementation::Integer.new(name: "#{name}_sysv_integer", | ||
| permissions: permissions) | ||
| @state = implementation::State.new(name: "#{name}_sysv_state", | ||
| permissions: permissions) | ||
| end | ||
|
|
||
| def acquire | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ module Simple | |
| class Integer #:nodoc: | ||
| attr_accessor :value | ||
|
|
||
| def initialize | ||
| def initialize(**) | ||
| reset | ||
| end | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| require 'forwardable' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you need this now? |
||
|
|
||
| module Semian | ||
| module Simple | ||
| class SlidingWindow #:nodoc: | ||
|
|
@@ -10,7 +12,7 @@ class SlidingWindow #:nodoc: | |
| # like this: if @max_size = 4, current time is 10, @window =[5,7,9,10]. | ||
| # Another push of (11) at 11 sec would make @window [7,9,10,11], shifting off 5. | ||
|
|
||
| def initialize(max_size:) | ||
| def initialize(max_size:, **) | ||
| @max_size = max_size | ||
| @window = [] | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| module Semian | ||
| module Simple | ||
| class State #:nodoc: | ||
| def initialize | ||
| def initialize(**) | ||
| reset | ||
| end | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| module Semian | ||
| module SysV | ||
| class Integer < Semian::Simple::Integer #:nodoc: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are you inheriting from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's to support a fallback in case SysV isn't activated for some reason. The C api will replace all the useful methods, like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a too unexpected way to provide the fallback. The fallback shouldn't be the SysV classes changing their implementation with
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point, isn't it easier to write this entire thing in C? You don't need the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, the issue is whether the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I meant with inheritance from C, but that probably doesn't give you anything and gives more poor visibility. |
||
| include SysVSharedMemory | ||
|
|
||
| def initialize(name:, permissions:) | ||
| data_layout = [:int] | ||
| super() unless acquire_memory_object(name, data_layout, permissions) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| module Semian | ||
| module SysVSharedMemory #:nodoc: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haven't had the opportunity to review this yet, but looks quite complicated at first glance
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most of the complications is in the C code, so read the PR description for a rundown of what happens. |
||
| @type_size = {} | ||
| def self.sizeof(type) | ||
| size = (@type_size[type.to_sym] ||= (respond_to?(:_sizeof) ? _sizeof(type.to_sym) : 0)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's for memoizing the sizes after it's read once from C. I don't want to hardcode sizes, they differ by platform.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with that, but I don't see why you need the local variable when you have the class instance variable.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh. For raising the TypeError. Maybe that should be moved to C instead? |
||
| raise TypeError.new("#{type} is not a valid C type") if size <= 0 | ||
| size | ||
| end | ||
|
|
||
| def self.included(base) | ||
| def base.do_with_sync(*names) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @byroot probably the most interesting change that you could provide an opinion for is this function here. I call it from C to replace a regular accessor or setter that accesses shared memory and is not a atomic call, and wrap it in a I just want to make sure there are no disagreements here. Relevant C code is and
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer to avoid this kind of monkey patching. I'd rather prefer have a noop
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While looking over the C code base, one of the scarier things was that because I provided the Here's the what a return rb_ensure(semian_shm_object_synchronize_with_block, self, semian_shm_object_synchronize_restore_lock_status, (VALUE)&status);For reference: https://github.com/Shopify/semian/pull/54/files#diff-905e62ddb202d77b001ca1dbac3cbf06R310 Essentially each function needed a wrapper, and probably also some custom struct to overcome the one-argument limit of the function With regards to the noop
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kyewei I think what he means with noop syncronize is that the SysV implementations continue to inherit from class Semian::Simple::Integer
# .. stuff
def increment(delta)
synchronize { value += delta }
end
private
def synchronize
yield
end
# more stuff
endWhen this mixin is included, it just overrides it. The implementation can then be simplified when all the fallback checking is done at boot-time because you can just override it in C, and not have it in Ruby.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see what you're saying, but that also doesn't work in protecting lets say.. For example, in code, def increment(val=1)
synchronize { self.value += 1 } # OK
end
def value
#how do you put a synchronize in here?
# you can't do self.value, that would just do recursion and crash
@value
end |
||
| names.each do |name| | ||
| new_name = "#{name}_inner".freeze.to_sym | ||
| alias_method new_name, name | ||
| private new_name | ||
| define_method(name) do |*args, &block| | ||
| synchronize do | ||
| method(new_name).call(*args, &block) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can still kill this meta-programming by the no-op
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #64 (comment)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand your concern, but can't you just make
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue is that if you define I don't think the no-op synchronize works at all in this situation, is what I'm saying
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll try to put what I meant into code, I think all you need to do is make sure you don't try to grab the mutex again if you nest (with the mutex here being the shared memory one, I'm just using a pure Ruby example): class A
def value
synchronize { @value }
end
private
def synchronize
yield
end
end
class B < A
def initialize
@mutex = Mutex.new
end
def increment(val = 1)
synchronize { self.value += val }
end
private
def synchronize
return yield if @nested
@mutex.synchronize {
@nested = true
yield
}
end
end
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But why do you have to do it for any other method than
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For the SlidingWindow ones: You could make the case to remove the synchronize from the accessors, but why would you make it less safe, for example.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The things I still don't understand why you can't just do this for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because you'd still have the same problem for each of those Theres no good way in Ruby to call a function before hand, call contents, and call an ensure after without some metaprogramming or writing it explicitly everywhere.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's discuss this IRL after lunch |
||
|
|
||
| def semid | ||
| -1 | ||
| end | ||
|
|
||
| def shmid | ||
| -1 | ||
| end | ||
|
|
||
| def synchronize(&block) | ||
| if respond_to?(:_synchronize) && @using_shared_memory | ||
| return _synchronize(&block) | ||
| else | ||
| yield if block_given? | ||
| end | ||
| end | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Example of a function that could probably just be implemented in C and would benefit from the simplification of not doing run-time fallbacks. |
||
|
|
||
| alias_method :transaction, :synchronize | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's this for?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had another name for it originally,
begin
lock_semaphore()
yield
ensure
restore_semaphore_state()
endexcept it is massively verbose and spread across 2 functions since C does |
||
|
|
||
| def destroy | ||
| if respond_to?(:_destroy) && @using_shared_memory | ||
| _destroy | ||
| else | ||
| super | ||
| end | ||
| end | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why wouldn't the C implementation just override this (given the driver check is done at boot time). |
||
|
|
||
| private | ||
|
|
||
| def shared? | ||
| @using_shared_memory | ||
| end | ||
|
||
|
|
||
| def acquire_memory_object(name, data_layout, permissions) | ||
| return @using_shared_memory = false unless Semian.semaphores_enabled? && respond_to?(:_acquire) | ||
|
||
|
|
||
| byte_size = data_layout.inject(0) { |sum, type| sum + ::Semian::SysVSharedMemory.sizeof(type) } | ||
|
||
| raise TypeError.new("Given data layout is 0 bytes: #{data_layout.inspect}") if byte_size <= 0 | ||
| # Calls C layer to acquire/create a memory block, calling #bind_initialize_memory_callback in the process, see below | ||
| _acquire(name, byte_size, permissions) | ||
| @using_shared_memory = true | ||
| end | ||
|
|
||
| def bind_initialize_memory_callback | ||
| # Concrete classes must implement this in a subclass in C to bind a callback function of type | ||
| # void (*initialize_memory)(size_t byte_size, void *dest, void *prev_data, size_t prev_data_byte_size); | ||
| # to location ptr->initialize_memory, where ptr is a semian_shm_object* | ||
| # It is called when memory needs to be initialized or resized, possibly using previous memory | ||
|
||
| raise NotImplementedError | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| module Semian | ||
| module SysV | ||
| class SlidingWindow < Semian::Simple::SlidingWindow #:nodoc: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto on: Wouldn't this be better to just have implemented in C? |
||
| include SysVSharedMemory | ||
|
|
||
| def initialize(max_size:, name:, permissions:) | ||
| data_layout = [:int, :int] + [:long] * max_size | ||
| super(max_size: max_size) unless acquire_memory_object(name, data_layout, permissions) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| require 'forwardable' | ||
|
|
||
| module Semian | ||
| module SysV | ||
| class State < Semian::Simple::State #:nodoc: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made a couple of comments here, in general I think you can simplify this implementation by sharing more functionality from |
||
| include SysVSharedMemory | ||
| extend Forwardable | ||
|
|
||
| def_delegators :@integer, :semid, :shmid, :synchronize, :transaction, | ||
| :shared?, :acquire_memory_object, :bind_initialize_memory_callback | ||
| private :shared?, :acquire_memory_object, :bind_initialize_memory_callback | ||
|
|
||
| def initialize(name:, permissions:) | ||
| @integer = Semian::SysV::Integer.new(name: name, permissions: permissions) | ||
| initialize_lookup | ||
| end | ||
|
|
||
| def open | ||
| self.value = :open | ||
| end | ||
|
|
||
| def close | ||
| self.value = :closed | ||
| end | ||
|
|
||
| def half_open | ||
| self.value = :half_open | ||
| end | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it was @byroot that said something about not liking a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't want
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome. |
||
|
|
||
| def reset | ||
| close | ||
| end | ||
|
||
|
|
||
| def destroy | ||
| reset | ||
| @integer.destroy | ||
| end | ||
|
||
|
|
||
| def value | ||
| @num_to_sym.fetch(@integer.value) { raise ArgumentError } | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def value=(sym) | ||
| @integer.value = @sym_to_num.fetch(sym) { raise ArgumentError } | ||
| end | ||
|
|
||
| def initialize_lookup | ||
| # Assume symbol_list[0] is mapped to 0 | ||
| # Cannot just use #object_id since #object_id for symbols is different in every run | ||
| # For now, implement a C-style enum type backed by integers | ||
|
|
||
| @sym_to_num = {closed: 0, open: 1, half_open: 2} | ||
| @num_to_sym = @sym_to_num.invert | ||
|
||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| require 'test_helper' | ||
|
|
||
| class TestSysVInteger < MiniTest::Unit::TestCase | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about proving the integer is shared? |
||
| CLASS = ::Semian::SysV::Integer | ||
|
|
||
| def setup | ||
| @integer = CLASS.new(name: 'TestSysVInteger', permissions: 0660) | ||
| @integer.reset | ||
| end | ||
|
|
||
| def teardown | ||
| @integer.destroy | ||
| end | ||
|
|
||
| include TestSimpleInteger::IntegerTestCases | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| require 'test_helper' | ||
|
|
||
| class TestSysVSlidingWindow < MiniTest::Unit::TestCase | ||
| CLASS = ::Semian::SysV::SlidingWindow | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a little bit of a nitpick, but I'd call this |
||
|
|
||
| def setup | ||
| @sliding_window = CLASS.new(max_size: 6, | ||
| name: 'TestSysVSlidingWindow', | ||
| permissions: 0660) | ||
| @sliding_window.clear | ||
| end | ||
|
|
||
| def teardown | ||
| @sliding_window.destroy | ||
| end | ||
|
|
||
| include TestSimpleSlidingWindow::SlidingWindowTestCases | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't there be a test proving it's shared and that access is syncronized where needed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't add the shared tests in the commit, the tests and build would fail. Do you want me to include it?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, you make a good point, let's wait with them—but make sure to not forget them. We can't merge this unless it's green. |
||
|
|
||
| private | ||
|
|
||
| include TestSimpleSlidingWindow::SlidingWindowUtilityMethods | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| require 'test_helper' | ||
|
|
||
| class TestSysVState < MiniTest::Unit::TestCase | ||
| CLASS = ::Semian::SysV::State | ||
|
|
||
| def setup | ||
| @state = CLASS.new(name: 'TestSysVState', | ||
| permissions: 0660) | ||
| @state.reset | ||
| end | ||
|
|
||
| def teardown | ||
| @state.destroy | ||
| end | ||
|
|
||
| include TestSimpleState::StateTestCases | ||
|
|
||
| def test_will_throw_error_when_invalid_symbol_given | ||
| # May occur if underlying integer gets into bad state | ||
| integer = @state.instance_eval "@integer" | ||
| integer.value = 100 | ||
| assert_raises ArgumentError do | ||
| @state.value | ||
| end | ||
| assert_raises ArgumentError do | ||
| @state.open? | ||
| end | ||
| assert_raises ArgumentError do | ||
| @state.half_open? | ||
| end | ||
| assert_raises ArgumentError do | ||
| @state.closed? | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implementationshould probably be passed as an argument.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't quite understand how the implementation can be hardcoded. How does the fallback work then? It sounds like in that case there's no test to make sure it goes to the Simple fallback when SysV is not available / disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How I did this before was that the
SysV::classes can themselves function as the fallback, since they inherit from theSimple::classes. Distinguishing between the two states was the reason I had@using_shared_memoryas a variable.I guess what you're imagining is more of a