-
Notifications
You must be signed in to change notification settings - Fork 79
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
EDIT: This branch is the iteration target. Implementing a per-host circuit breaker state using shared memory and semaphores #54
base: main
Are you sure you want to change the base?
Conversation
.gitignore
Outdated
/lib/semian/*.so | ||
/lib/semian/*.bundle | ||
/lib/semian*/*.so | ||
/lib/semian*/*.bundle |
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.
Wouldn't /lib/**/*.so
work here?
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.
This shouldn't be necessary if you move to a single .so
.
ext/semian_cb_data/semian_cb_data.c
Outdated
shared_cb_data *shm_address; | ||
} semian_cb_data; | ||
|
||
static int system_max_semaphore_count; |
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.
This is never used.
This needs some tests around (at least):
Other than that I'm assuming you're borrowing from the existing test suite? |
Agree. We also don't need to use |
ext/semian_cb_data/semian_cb_data.c
Outdated
if (TYPE(id) != T_SYMBOL && TYPE(id) != T_STRING) | ||
rb_raise(rb_eTypeError, "id must be a symbol or string"); | ||
if (TYPE(size) != T_FIXNUM /*|| TYPE(size) != T_BIGNUM*/) | ||
rb_raise(rb_eTypeError, "expected integer for arr_max_size"); |
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.
The name arr_max_size
is the internal name, it won't make much sense for the Ruby caller.
…ared memory and semaphores Shared memory stores the space of int+int+double[], corresponding to successes, array length, and the errors array itself
Remove second Ruby extension Provide fallback Ruby implementation of Sliding Window Sliding window resizes if new size is given and Semian restarts New worker added does not reset shared memory Use integers in ms for time, not floats
…herited subclasses
… Rubocop, added correctness testing with FakeSysV classes, introduce Semian.extension_loaded
…ation (#destroy, #shared), use #run_test_with_x_classes
…er using options, mutex for non-sysV
…ule, remove "Atomic" naming, change execute_atomically to synchronize
9957a65
to
4896c3c
Compare
0ccf771
to
1aa5cf4
Compare
…ck functions, trimmed C code, introduced a define_method_with_synchronize
60d9687
to
a35d0d7
Compare
@sirupsen @csfrancis @byroot for review, suggest refactoring if needed!
Implementation details:
Shared memory stores the space of
int
+int
+double[]
, corresponding to@successes
,@errors.count
, and the@errors
array itself (stored as doubles). The array has a max size of@errors.count
since the Ruby code ensures the array never grows above that size.semian.c
.semian.c
,semian_resource_free()
doesn't clear the semaphores, instead leaving them to be cleared on reboot.PR v2 (10/06/2015, up to hash 1f0960b...)
I did quite a lot of refactoring, so here's a detailed overview.
Semian::SharedMemoryObject
has two subclasses,Semian::AtomicInteger
andSemian::SlidingWindow
. I took suggestions to abstract the memory management further so thatSemian::CircuitBreaker
would only be using instances of the specific subclasses. I also added header files so code can be shared.Semian::AtomicEnum
subclassesSemian::AtomicInteger
to provide shared memory forCircuitBreaker
's@state
. It runs like a traditional C-style enum type thats backed by integers. It's used in@state
's:closed
,:open
,:half_open
SharedMemoryObject
provides and manages the semaphore, shared memory, and the subclasses implement the use cases for the memory. Constructor calls look like this:SharedMemoryObject.new("name", [:int, :int, :long, :long], 0660)
and specify the memory structure. The associated C structure has a reference to function of typevoid (*object_init_fn)(size_t, void *, void *, size_t, int)
so a custom data initializer can be used for each typeextend Forwardable
for delegating and decorating but that would introduce some complications with binding C functions to objects (things likeAtomicInteger#value
would need an extra argument to accessSharedMemoryObject
functions)SharedMemoryObject#execute_atomically
to allow custom actions such as those found inCircuitBreaker#push_time
without providing#lock
and#unlock
explicitlySlidingWindow
now works properly. The following scenarios resize:size=n
; worker 2 is created, requesting memory ofsize!=n
; both workers will use new size, with data copied over and reinitialized usingobject_init_fn()
. This occurs when you restart workers with new configurations#resize_to
is called with new size on a worker, every worker will resize to new sizeI'm looking for suggestions for how to structure the fallback ruby implementation so that both the fallback and the shared memory versions of
AtomicInteger
,SlidingWindow
, etc can coexist. Right now C code hooks and replaces them if semaphores are enabled, i.e. AtomicInteger is either semaphore-supported or it isn't, and theres no way to toggle between them.Also, if I missed anything, comment below.