-
Notifications
You must be signed in to change notification settings - Fork 80
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
Changes from all 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 |
---|---|---|
|
@@ -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' | ||
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: | ||
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 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 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 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 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 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: | ||
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 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) | ||
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 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 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 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
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. 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 | ||
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 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) 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 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 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 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 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. 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 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. 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? | ||
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 | ||
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 | ||
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 | ||
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. 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? 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: | ||
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: | ||
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 | ||
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 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 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 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 | ||
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 } | ||
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 | ||
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? |
||
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 |
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 | ||
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? 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? 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 | ||
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 |
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.
Huh? Only require
semian/semian
is semaphores are enabled?