-
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 5 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 |
|---|---|---|
|
|
@@ -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 |
|---|---|---|
| @@ -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,50 @@ | ||
| 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. |
||
| 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 | ||
| yield if block_given? | ||
|
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 not quite what I meant by a no-op synchronize, see my code-example in the other comment. This code should be in the |
||
| 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. |
||
|
|
||
| def destroy | ||
| super | ||
| 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 acquire_memory_object(*) | ||
| # Concrete classes must call this method before accessing shared memory | ||
| # If SysV is enabled, a C method overrides this stub and returns true if acquiring succeeds | ||
| false | ||
| 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 | ||
|
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 feel like the name could be better then.
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. As they get increasingly simpler as we iterate them closer to the Simple implementation, do we even need these exposed to 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. If nothing else, this needs to be clearly stated to be overriden, etc. Also, isn't this an old diff? That |
||
| 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,31 @@ | ||
| 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 | ||
|
|
||
| SYM_TO_NUM = {closed: 0, open: 1, half_open: 2}.freeze | ||
| NUM_TO_SYM = SYM_TO_NUM.invert.freeze | ||
|
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. Declaring these in
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. Simple has the lookup hard-coded, as requested earlier. If other drivers want them, why wouldn't then just use
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. Because
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. Then there's going to be the question for why it's there, since it seems out of place in a class that hardcodes them. Then it leads to |
||
|
|
||
| def_delegators :@integer, :semid, :shmid, :synchronize, :acquire_memory_object, | ||
| :bind_initialize_memory_callback, :destroy | ||
| private :acquire_memory_object, :bind_initialize_memory_callback | ||
|
|
||
| def initialize(name:, permissions:) | ||
| @integer = Semian::SysV::Integer.new(name: name, permissions: permissions) | ||
| 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.
|
||
|
|
||
| def value | ||
| NUM_TO_SYM.fetch(@integer.value) { raise ArgumentError } | ||
|
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. If you did |
||
| end | ||
|
|
||
| private | ||
|
|
||
| def value=(sym) | ||
| @integer.value = SYM_TO_NUM.fetch(sym) { raise ArgumentError } | ||
| 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