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

Entire SysV::Integer code and associated shared libraries #67

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kyewei
Copy link
Contributor

@kyewei kyewei commented Nov 23, 2015

Suggested to code review it horizontally (all the way through for one feature set instead of layer by layer), since it's easier to see the flow of code execution like that, all tests can be introduced, and tests don't fail because relevant code isn't included.

Old massive PR: #54
Small PR 1 (merged): #62
Small PR 2 (with description of code flow, closed): #64

Possibly outstanding issues

  • Using the do_with_sync helper + automatic wrapping of methods in synchronize:
    • I explained to Simon how there is no good way about doing this. We want to make sure locks are being unlocked after use, and handle situations where exceptions are raised. The process was this:
    • We want calling function y() inside function x() to be safe even if y() throws errors or exits prematurely at different exit points
    • Either have to repeat restore_lock_state() functionality at all exit points (hard to maintain and debug) or use an ensure
    • An alternate approach that isn't good is having some set_value(), get_value() accessors, and only protect those, having all mutation functionality for a data structure route through that. But that doesn't work well with something like SysV::SlidingWindow, since <<, shift, clear, all set values and mutate. On that line of thinking, the pragmatic way to do that is to provide a [] and []= function. This is bad since (1) it further raises the complexity, you'd have to make sure all functionality route through those functions, (2) we want a SlidingWindow not an Array. It is generally safer to sweep across all methods and wrap all of them in one swift go than to pick apart specific ones.
    • So we use ensure, but C's ensure syntax is terrible, since it utilizes callback functions in the form of rb_ensure(body_fn, body_fn_arg, ensure_fn, ensure_fn_arg) to emulate the begin; body_code; ensure; ensure_code; end form of Ruby. Two bad things:
      • Can only pass one argument, so you need random custom structs to contain your multiple arguments,
      • Forces each function to require at least one wrapper function, so that the wrapper can call the inner body_fn; Bad for code organization, and duplicate stub code everywhere
    • So instead use something in the form of synchronize { actual_method() } to wrap every function
      • Tried out different ways of accomplishing this:
      • Do an alias_method-chain thing current implementation
        • Tried to do this in C, passing around C functions as blocks and other complications (not being able to get a self from block calls (wtf)) made this impossible
        • Get a helper in Ruby (what do_with_sync) to do the hard part and make it succinct what I did
      • Do some prepend magic: I tried this, the call to rb_call_with_super always fails for some reason, might be the discrepancy between method definitions and variable argument counts
    • TLDR: Making it safe is between rock and hard place
      • Rock: protecting all the exit paths yourself, hard to maintain, still might fail, might memory leak if malloc is used
      • Hard place: doing ensures causes (1) either stub code everywhere, or (2) forcing code through a limited amount of setters and getters (not scalable and has other issues), or (3 current) slightly awkward method aliasing magic.

Check the bottom of the PR #64 's description for an overview of how accessor/setter function and the acquire flow works

@sirupsen

My god why are these PR descriptions so long

@miry
Copy link
Contributor

miry commented Sep 27, 2022

@byroot @bmansoob @dalehamel I think we don't need semaphores as distributed storage for Circuit breaker, after switching to Thread base web server like Puma and run everything in containers should limit circuit breaker to single process per host.

I have not tested if semaphores shared between containers,
but I would expect it is not or requires a special permission.

Also semaphores limit implementation to Linux only, that made harder development and using semians in MacOS or Windows.

@miry miry added the Semian label Sep 27, 2022
@byroot
Copy link
Contributor

byroot commented Sep 27, 2022

after switching to Thread base web server like Puma

That's not going to happen for various reasons. And even if we did, we'd at best run two threads per process, rendering an in-memory semaphore entirely useless.

@miry
Copy link
Contributor

miry commented Sep 28, 2022

That's not going to happen for various reasons. And even if we did, we'd at best run two threads per process, rendering an in-memory semaphore entirely useless.

I would try to write my understandings of required changes:

  • This PR introduces just SySVState (share state on same host),
  • There is still require another PR to create SySVState implementation and update configuration where developer can pick any of the current solutions: Simple, ThreadSafe, and SySVState.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants