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

Proposed SysV structures and Mixin functionality #64

Closed
wants to merge 6 commits into from
Closed
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
7 changes: 6 additions & 1 deletion lib/semian.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.semaphores_enabled? ? ::Semian::SysV : ::Semian::Simple,
)
resource = Resource.new(name, tickets: tickets, permissions: permissions, timeout: timeout)
resources[name] = ProtectedResource.new(resource, circuit_breaker)
Expand Down Expand Up @@ -160,6 +161,10 @@ def resources
require 'semian/simple_integer'
require 'semian/simple_state'
if Semian.semaphores_enabled?
require 'semian/sysv_shared_memory'
require 'semian/sysv_sliding_window'
require 'semian/sysv_integer'
require 'semian/sysv_state'
require 'semian/semian'
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh? Only require semian/semian is semaphores are enabled?

else
Semian::MAX_TICKETS = 0
Expand Down
12 changes: 8 additions & 4 deletions lib/semian/circuit_breaker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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}_sliding_window",
permissions: permissions)
@successes = implementation::Integer.new(name: "#{name}_integer",
permissions: permissions)
@state = implementation::State.new(name: "#{name}_state",
permissions: permissions)
end

def acquire
Expand Down
2 changes: 1 addition & 1 deletion lib/semian/simple_integer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ module Simple
class Integer #:nodoc:
attr_accessor :value

def initialize
def initialize(**)
reset
end

Expand Down
4 changes: 3 additions & 1 deletion lib/semian/simple_sliding_window.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
require 'forwardable'
Copy link
Contributor

Choose a reason for hiding this comment

The 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:
Expand All @@ -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
Expand Down
11 changes: 6 additions & 5 deletions lib/semian/simple_state.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
module Semian
module Simple
class State #:nodoc:
def initialize
def initialize(**)
reset
end

attr_reader :value
attr_accessor :value
private :value=

def open?
value == :open
Expand All @@ -20,15 +21,15 @@ def half_open?
end

def open
@value = :open
self.value = :open
end

def close
@value = :closed
self.value = :closed
end

def half_open
@value = :half_open
self.value = :half_open
end

def reset
Expand Down
12 changes: 12 additions & 0 deletions lib/semian/sysv_integer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
module Semian
module SysV
class Integer < Semian::Simple::Integer #:nodoc:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you inheriting from Simple::Integer? Won't the implementation be completely different?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 #value, #value=, #increment, #reset

Copy link
Contributor

Choose a reason for hiding this comment

The 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 super and inheritance, it should be done at boot-time. This is dangerous and would be incredibly difficult to debug.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 unless below anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, the issue is whether the SysV structure should be able to itself fallback to a Simple's functionality. That's what I did here to make that work.
I don't see a justification for writing it in C if it's doing exactly what it's doing here, except in more verbose C.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
50 changes: 50 additions & 0 deletions lib/semian/sysv_shared_memory.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
module Semian
module SysVSharedMemory #:nodoc:
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 synchronize. I wrap all the functions with this, so it's essentially a do_with_sync :value=, :value, :reset, :increment and do_with_sync :size=, :max_size, :<<, :push, :pop etc

I just want to make sure there are no disagreements here.

Relevant C code is
https://github.com/Shopify/semian/blob/circuit-breaker-per-host/ext/semian/semian_shared_memory_object.c#L319-L324
where define_method_with_synchronize calls do_with_sync

and
https://github.com/Shopify/semian/pull/54/files#diff-99c3116b9c2c62cc89a4ae4cc84fadaaR262
which is where define_method_with_synchronize is used in place of rb_define_method

Copy link
Contributor

Choose a reason for hiding this comment

The 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 sychronize method in Simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 lock() and unlock() as separate functions, it was very difficult to track where I had to re-unlock, since errors could occur in the middle of a function, and then you'd have to return right there, and insert an unlock() there. It just isn't safe to have many exit points sprawled all over the code. So I then opted to write ensures in, but doing so manually would add twice the number of functions and make things very manual.

Here's the what a begin-ensure would look like:

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 rb_ensure. This naturally lead to me doing it in Ruby instead since it doesn't have these limitations, which is why synchronize does what it does. IMO it's not a big deal since it only does it once per class, reduces code and complexity by a bit. Maybe using prepend with its use of super would make it cleaner than doing what I do currently, which is having an #{method_name}_inner as the original and ##{method_name} as the synchronized version.

With regards to the noop synchronize, do you mean something more along the lines of (1) , with SharedMemoryObject -> Simple::Integer -> SysV::Integer ? Otherwise I don't understand what you mean by noop synchronize. Unless you mean just having one there to keep the interfaces the same?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 Simple::Integer and friends, which then look like this:

class Semian::Simple::Integer
  # .. stuff
  def increment(delta)
    synchronize { value += delta }
  end

  private
  def synchronize
    yield
  end
  # more stuff
end

When 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.. Semian::SysV::Integer#value. If the #value is not protected in C code, and it directly replaces the SysV::Integer, there is no entry point to override and replace with synchronize { ... } except through what do_with_sync does. If you do protect in C, well, that goes back to the problem of making 2x the functions to call rb_ensure(callback, arg1, ensure_fn, arg2)

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
Copy link
Contributor

Choose a reason for hiding this comment

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

You can still kill this meta-programming by the no-op synchronize in the Simple implementation, as showed with code in another comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#64 (comment)
You probably didn't see it, since it was on an old diff. I responded saying how a no-op synchronize doesn't work in all the cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand your concern, but can't you just make synchronize detect whether it's being recursed and in that case not grab the lock again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is that if you define #value to return @value in Simple, your ability to share that with SysV is out since the SysV ones don't define @value. If you define #value to return self.value, well, then you have infinite looping.

I don't think the no-op synchronize works at all in this situation, is what I'm saying

Copy link
Contributor

Choose a reason for hiding this comment

The 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 

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#increment, gets and sets value
#reset delegates to #value=
#value= should definitely be atomic

For the SlidingWindow ones:
"size", "max_size", "resize_to", "<<", "push", "pop", "shift", "unshift", "clear", "first", "last", either they are accessors or they modify the array, so they are necessary.

You could make the case to remove the synchronize from the accessors, but why would you make it less safe, for example.
It makes sense to just make it uniform from then on.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 #value and reuse the implementation for the other ones, so you don't need this ugly meta programming that makes it hard to grep for and grok what's going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because you'd still have the same problem for each of those <<, pop, shift,clear etc. UNLESS you provide some uniform [] and make everything delegate to that, but I distinctly remember the motivation for not having [] that was to make it a SlidingWindow, not an Array

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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?
Copy link
Contributor

Choose a reason for hiding this comment

The 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 Simple classes, not here where it should do the right thing.

end
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like the name could be better then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 acquire_memory_object is old. The current one just returns false

raise NotImplementedError
end
end
end
12 changes: 12 additions & 0 deletions lib/semian/sysv_sliding_window.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
module Semian
module SysV
class SlidingWindow < Semian::Simple::SlidingWindow #:nodoc:
Copy link
Contributor

Choose a reason for hiding this comment

The 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
31 changes: 31 additions & 0 deletions lib/semian/sysv_state.rb
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:
Copy link
Contributor

Choose a reason for hiding this comment

The 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 Semian::Simple::State. You may have to modify that class a little, but I think that's fine.

include SysVSharedMemory
extend Forwardable

SYM_TO_NUM = {closed: 0, open: 1, half_open: 2}.freeze
NUM_TO_SYM = SYM_TO_NUM.invert.freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

Declaring these in SIMPLE and validating against them might be more appropriate, as other drivers would likely want these.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Semian::SysV::State::SYM_TO_NUM, etc?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because Simple is the canonical implementation everything inherits from, not SysV.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 why is it hardcoded and stuff happens.


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
Copy link
Contributor

Choose a reason for hiding this comment

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

super instead of reset?


def value
NUM_TO_SYM.fetch(@integer.value) { raise ArgumentError }
Copy link
Contributor

Choose a reason for hiding this comment

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

If you did alias_method :to_i, :value and always called value.to_i in Simple::State, you wouldn't need to override these. Duck typing hard!

end

private

def value=(sym)
@integer.value = SYM_TO_NUM.fetch(sym) { raise ArgumentError }
end
end
end
end
2 changes: 1 addition & 1 deletion test/simple_state_test.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'test_helper'

class TestSimpleEnum < MiniTest::Unit::TestCase
class TestSimpleState < MiniTest::Unit::TestCase
CLASS = ::Semian::Simple::State

def setup
Expand Down
16 changes: 16 additions & 0 deletions test/sysv_integer_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
require 'test_helper'

class TestSysVInteger < MiniTest::Unit::TestCase
Copy link
Contributor

Choose a reason for hiding this comment

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

What about proving the integer is shared?

KLASS = ::Semian::SysV::Integer

def setup
@integer = KLASS.new(name: 'TestSysVInteger', permissions: 0660)
@integer.reset
end

def teardown
@integer.destroy
end

include TestSimpleInteger::IntegerTestCases
end
22 changes: 22 additions & 0 deletions test/sysv_sliding_window_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
require 'test_helper'

class TestSysVSlidingWindow < MiniTest::Unit::TestCase
KLASS = ::Semian::SysV::SlidingWindow

def setup
@sliding_window = KLASS.new(max_size: 6,
name: 'TestSysVSlidingWindow',
permissions: 0660)
@sliding_window.clear
end

def teardown
@sliding_window.destroy
end

include TestSimpleSlidingWindow::SlidingWindowTestCases
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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
35 changes: 35 additions & 0 deletions test/sysv_state_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
require 'test_helper'

class TestSysVState < MiniTest::Unit::TestCase
KLASS = ::Semian::SysV::State

def setup
@state = KLASS.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