Skip to content

Allowing access to previous LSpaces for EM error handler #6

Closed
ebroder opened this Issue Jan 3, 2013 · 16 comments

2 participants

@ebroder
ebroder commented Jan 3, 2013

I'd be happy to write code for this, but mostly wanted to talk about design/semantics/etc. first

We currently have an internal implementation of a system for dynamic variable scoping. One of the places we use it heavily is our custom EventMachine error handler. In our implementation, the error handler runs with the environment of the code that just raised, letting us inspect various aspects of the code's state to dispatch exceptions.

Right now, LSpace wraps EM.error_handler such that the error handler runs in LSpace that was active when it was registered. That seems, frankly, more semantically correct than what we do. But I'd still like some way to access the LSpace of the block that just raised.

My current thinking is to take the pre-raise LSpace and put it somewhere in the LSpace of the error handler. That seems almost doable from an around_filter, but I'm running into two issues:

  • AFAICT it's currently impossible to register an around_filter for all present and future LSpaces. In particular, even if I could grab the root LSpace for the main thread, I can't modify the around_filters for LSpaces created for other threads in the future. Because I'm using EM, I don't actually expect to care about LSpaces in other threads, but seems worthwhile to chase down correctness here.

    My best idea is to have some sort of concept of "global around filters". I can't tell whether or not that's a horrible idea in practice.

  • Even if I've caught an exception within the LSpace that just raised, I can't figure out how to inject that into the LSpace of the error handler.

Any thoughts on any of that?

@ConradIrwin
Owner

The idea of having access to multiple lspaces has come up before in discussion with @halorgium about lspace with celluloid. It seems that it's often useful to be able to enter an lspace that is the union of two (I.e. the one that was active when the exception handler was created and the one that was active when the exception was raised). In celluloid we wanted the one that was active when the actor was created and the one that was active when the message was sent.

The obvious problem is that it's somewhat undefined what should happen if you enter two lspaces at the same time... That said I think it's exactly the same problem as multiple inheritance, so we can fix it in the same way that Python does. My main concern about this is that the lspace hierarchy is going to become pretty mind bending pretty quickly, so it may not be easy to intuit which variables come from where. That may not be a problem in real life though.

Alternatively the less automatic approach that you outlined seems simpler to understand, where we explicitly allow access to the 'previous' lspace, so for the exception handler you can access the lspace in which the exception was raised, and for celluloid you can access the lspace from which the message was sent while still accessing the lspace stored at actor creation time by default. I'm not a fan of this idea on reflection as it may be very fragile if you refactor some code into a block then it may need to access the previous previous lspace instead of just the previous lspace.

There might be a cleaner solution of we explicitly recognize two kinda of lspace, the one stored at define time and the one active at runtime; but I can't see it clearly.

For the specific case of exception handling, what we do is insert an around_filter into all server connections that rescues exceptions, that way the handler knows which connection it's attached to, and we very rarely hit the global em error handler. I'm not sure whether that works for you, it certainly just side steps the main issue!

The main problem with the around filter abstraction is, as you notce the problem of being unable to wrap any currently running lspaces. We should perhaps split an around filter into 2. before, and after, where after is called with the return value and any raised exception. That might be a good thing to do anyway, but would it solve all your problems?

Sorry for the rambling, let me know if I need to clarify anything.

@ebroder
ebroder commented Feb 17, 2013

Ok, I thought that "use an around_filter instead" was going to be enough to solve this problem, but I'm really struggling with the process for attaching new around filters to the global LSpace.

The rough setup is that I want to create an around_filter that gets called any time the EM reactor calls into my code. But if I call LSpace.around_filter before calling EM.run, the filter never gets called because of LSpace's double-filtering protections.

I also can't do the standard shenanigans of

  lspace = LSpace.new
  lspace.around_filter {|&block| block.call rescue nil}
  lspace.enter { "setup callbacks here" }

because all of that setup happens within Thin. (I think for EM in particular I would have to do something like EM.run { lspace.enter { "setup callbacks here" } }, but I'm not totally sure.

I'm having a slightly hard time wrapping my head around this exactly, and I can't quite tell if I'm missing something obvious. Am I?

@ebroder
ebroder commented Feb 17, 2013

Oh wait, might have just gotten something by actually taking a look at em-monitor - I think monkey-patching EM.run will actually get me the properties I want.

@ConradIrwin
Owner

I don't think you're missing something obvious, though it would be nice if you were.

The approach that em-monitor takes is to Monkeypatch em-run to do exactly what you suggest above: https://github.com/ConradIrwin/em-monitor/blob/master/lib/em-monitor.rb#L13

I'm open to suggestions for how to make this better — maybe we can somehow automatically make EM::run take care of this for everyone.

@ConradIrwin
Owner

Concurrrrency!

@ConradIrwin
Owner

What would you think of adding this to lspace?

class << EM
  alias_method :run_without_lspace, :run
  def run do
    lspace = LSpace.current
    LSpace.clean do
      run_without_lspace do
        lspace.enter do
          yield
        end
      end     
  end
end

It's a bit ugly, but I think it gives everyone the semantics they want. (I actually had a bug in the specs because I had naively assumed that this is what would happen: fc33615 )

@ebroder
ebroder commented Feb 17, 2013

Jeez that makes my head hurt but it does kind of seem like it might be the right idea.

@ebroder
ebroder commented Feb 17, 2013

Although it seems a bit strange that this will only work within the EM reactor. But maybe that's OK

@ebroder
ebroder commented Feb 17, 2013

Err, in the sense that it wouldn't work if you were using LSpace outside of an EM world.

I think I'm mostly digging here, and this behavior is probably close enough to what people want to be good enough.

@ConradIrwin
Owner

What do you mean by "this"?

@ebroder
ebroder commented Feb 17, 2013

Not totally sure. Maybe just that EM.run is special in terms of how it manipulates LSpace state.

@ConradIrwin
Owner

Oh, ok. Yeah I think know what you mean — there are probably similar things with other frameworks, but I don't know celluloid well enough to have spotted it yet :). EM::run is kind of special because it hands control of the thread over to the reactor.

@ebroder
ebroder commented Feb 17, 2013

I'm slightly suspicious that the need for that kind of hoop-jumping is an indicator of a problem with the abstraction, but I also haven't thought of anything that would be better, so I think probably best to just merge this so we can get the desired functionality, and if a better abstraction appears to someone, then you can reconsider.

@ConradIrwin
Owner

Ok, pushed as v0.8 — let me know how it goes.

@ebroder
ebroder commented Feb 17, 2013

It definitely looks like I can get the functionality I need out of this, so I'll go ahead and close this issue.

@ebroder ebroder closed this Feb 17, 2013
@ConradIrwin
Owner

w00!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.