Hooks are invoked only on the first call

[charlie-wasp]

Hello!

Today I stumbled upon some strange issue. It seems that hooks (in my case after_actions) are invoked only on the first .call.

Here is, how my method call looks like in the Organizer class:

def self.call(chat_id:, user_to_ban_id:, voter:, vote: :for)
  with(
    chat_id: chat_id,
    user_to_ban_id: user_to_ban_id,
    voter: voter,
    vote: vote.to_sym,
    votes_storage: VotesStorage.new(user_to_ban_id),
  )
    .reduce(
      IncrementVotesCountAction,
      FetchVoteResultsAction,
      ResolveVotingAction,
    )
end

And in the Organizer.with I saw this:

light-service/lib/light-service/organizer.rb

Lines 19 to 31 in 2656002

	def with(data = {})
	VerifyCallMethodExists.run(self, caller(1..1).first)
	data[:_aliases] = @aliases if @aliases
	if @before_actions
	data[:_before_actions] = @before_actions.dup
	@before_actions = nil
	end
	if @after_actions
	data[:_after_actions] = @after_actions.dup
	@after_actions = nil
	end

So after first with call we set data[:_after_actions] properly, and then we nullify @after_actions. And it never came back, because of Macros.after_actions won't be called again. So on the next invocations of with, data[:_after_actions] won't be set.

Am I misunderstanding something here?

[adomokos]

Thank you for reporting it!

I added this spec to verify this issue. The :number in the result hash should be 1 (just like the one above it), but I expect 6 to get the test pass, as the callbacks are ignored. I'll have to rethink how the callbacks are kept for an organizer for subsequent calls.

The problem is that the callback is saved under the data[:_after_actions], however, the second time the Organizer is invoked, it creates a new LightService::Context and the :_after_actions and :_before_actions callbacks are lost. 😢

[adomokos]

I started thinking about this. One way to solve this is to save the action specific callbacks in the current thread, something like this:

Thread.current[:some_action_callbacks] = [proc1, proc2]

That way we could keep those callbacks around for multiple calls.

Thoughts?

[charlie-wasp]

@adomokos sorry for not answering, been busy 😞 First I should sort out a whole architecture, and then maybe I would be able to suggest something 😄

[charlie-wasp]

Quick (and therefore maybe stupid) question: why we should nullify @after_actions in the first place?

[klamontagne]

Hello, I was trying to use *_actions callbacks to add intrumentation to a light-service (via an included module) when I hit this bug/unexpected behavior. I was expecting to declare after_actions once in the class definition and have the instrumentation method run on every call (like around_each would help doing). The README example make it seem like this "static" implementation is intended. However, the spec uses it as a "dynamic" injection of the method (at runtime, before the call).

Maybe there should be a distinction between "static" callbacks and the current dynamic behavior.

Also, it seems the *_actions could be picked up by a service call running in another thread instead of the intended call. (not sure).

I will use around_each to achieve similar behavior for the moment.

[adomokos]

I added the *_actions callbacks as a quick help to provide ContextFactory functionality to Orchestrator organizers (#141). Logging did not get implemented properly and we found this issue with executing the callback only once.

@klamontagne - is your issue similar to the one described in #152?

[charlie-wasp]

@adomokos I've got a quick idea:

It seems that now there is no link back to the organizer from the action. How about incorporate it? With that, we'll be able to access callbacks from actions independently from context. So then we can stop nullify @after_actions in organizer (btw, why it is so now?), or use class variables (but possibly it's dangerous).

For me storing callbacks in context feels unnatural, like the fact that actions invoke callbacks. I believe, it should be the organizer's job, and organizer should know about callbacks, not actions themselves.

It's just my general thoughts after the quick run through the code.

What do you think about it?

[adomokos]

The reason I added it to the context is the fact that the Context carries the state of information, Actions are only behavior, Organizers are assembling the actions in the right order.
I always wanted to keep the Actions unaware of the surrounding context.

I am not a big fan of adding the callbacks to the LightService::Context either, I merely did it as that was the simplest thing that could possibly work at that time.

I need to think about this a bit more, maybe start a PR myself so you and others could comment on. I'll try to do it next week, when I'll be back in work-mode again.

If you have ideas on how to improve it, feel free to begin a PR and submit it early, so I could give you directions on where we should go with it. I'd hate to see you working on something for days/weeks and not merging it in the end.

Thanks!

[charlie-wasp]

Thank you for your reply @adomokos!

Organizers are assembling the actions in the right order
I always wanted to keep the Actions unaware of the surrounding context

These two points again lead me to the already expressed idea of moving callbacks invocation to Organizer or Reducer :) Therefore Organizer will have more control on actions flow (because callbacks are part of it), and Actions will have less knowledge about surroundings

I'll try to code something on the weekend but unfortunately can't guarantee it

[adomokos]

Yeah, this week is tough for me to put any serious effort into it. 2nd half of next week seems to be better.

[KelseyDH]

I recommend using RequestStore rather than Thread.current for saving this state, as it plays nicely with threaded servers.

[adomokos]

That is great, thank you for suggesting it.

However, that would add a dependency on rack and my heart already bleeds that LightService has dependency on ActiveSupport.

I'd much rather just remove the whole Thread.current from the code base if I could come up with something more elegant - without adding gem dependency.

[KelseyDH]

I mean you could make the rails part a separate gem that ties into this one, that overrides what it uses in a gem config variable. Or alternatively make sure you reset the Thread.current variables after it's finished executing so nothing persists longer than it should.

[adomokos]

Submit a PR, and I'll think about it ;-).