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

Better API for subscribers (dynamic and static) which are procs #155

Closed
paneq opened this Issue Oct 30, 2017 · 8 comments

Comments

2 participants
@paneq
Member

paneq commented Oct 30, 2017

Current API:

cancel_order = CancelOrder.new
event_store  = Rails.configuration.event_store

event_store.subscribe(
    -> (event) {
      Rails.logger.warn(event.inspect)
    },
    [OrderCancelled]
  ) do
  cancel_order.call(order_id, user_id)
end

Proposed API:

cancel_order = CancelOrder.new
event_store  = Rails.configuration.event_store

event_store.on(OrderCancelled) do |event|
  Rails.logger.warn(event)
end.during do
  cancel_order.call(order_id, user_id)
end

or

cancel_order = CancelOrder.new
event_store  = Rails.configuration.event_store

event_store.during do
  cancel_order.call(order_id, user_id)
end.on(OrderCancelled) do |event|
  Rails.logger.warn(event)
end.execute

Checklist

  • one shot subscribers
  • normal subscribers

@paneq paneq added the enhancement label Oct 30, 2017

@paneq

This comment has been minimized.

Show comment
Hide comment
@paneq

paneq Feb 21, 2018

Member

Today I was working on something and I wrote:

client.subscribe(->(ev){@ev = ev}, [ResTesting::OrderCreated])

and I realized again how non-idiomatic the whole situation is.

Here is another take on that API:

Long-term subscribers

event_store.subscribe(OrderCancelled, handler)

event_store.subscribe(OrderCancelled) do |event|
  Rails.logger.warn(event)
end

One-shot subscribers

event_store.within{ do_something }.subscribe(OrderCancelled, handler)

event_store.within do
  cancel_order.call(order_id, user_id)
end.subscribe(OrderCancelled) do |event|
  Rails.logger.warn(event)
end

I think within might be a better word than during. Capybara inspiration.
https://www.dropbox.com/s/tr60r54v6pw6qkx/Screenshot%202018-02-21%2022.35.37.png?dl=0

Member

paneq commented Feb 21, 2018

Today I was working on something and I wrote:

client.subscribe(->(ev){@ev = ev}, [ResTesting::OrderCreated])

and I realized again how non-idiomatic the whole situation is.

Here is another take on that API:

Long-term subscribers

event_store.subscribe(OrderCancelled, handler)

event_store.subscribe(OrderCancelled) do |event|
  Rails.logger.warn(event)
end

One-shot subscribers

event_store.within{ do_something }.subscribe(OrderCancelled, handler)

event_store.within do
  cancel_order.call(order_id, user_id)
end.subscribe(OrderCancelled) do |event|
  Rails.logger.warn(event)
end

I think within might be a better word than during. Capybara inspiration.
https://www.dropbox.com/s/tr60r54v6pw6qkx/Screenshot%202018-02-21%2022.35.37.png?dl=0

@paneq

This comment has been minimized.

Show comment
Hide comment
@paneq

paneq Feb 21, 2018

Member

I realized since within would be a new API returning a new object we could introduce it quite safely.

Member

paneq commented Feb 21, 2018

I realized since within would be a new API returning a new object we could introduce it quite safely.

@pawelpacana

This comment has been minimized.

Show comment
Hide comment
@pawelpacana

pawelpacana Feb 21, 2018

Member

Looks cool. How does it compare toAS::Notifications naming?

Member

pawelpacana commented Feb 21, 2018

Looks cool. How does it compare toAS::Notifications naming?

@paneq

This comment has been minimized.

Show comment
Hide comment
@paneq

paneq Feb 21, 2018

Member

As for changing the order of arguments for subscribe...

I am not sure how to go from

subscribe(subscriber, event_types, &proc)

to

subscribe(event_types, subscriber, &subscriber2)
  subscriber ||= subscriber2
end

I think we can recognize old API vs new API based on event_types if it responds to each (whether that's first or second argument). Let's hope not many handlers respond to each and this best-effort heuristic be good enough_

Member

paneq commented Feb 21, 2018

As for changing the order of arguments for subscribe...

I am not sure how to go from

subscribe(subscriber, event_types, &proc)

to

subscribe(event_types, subscriber, &subscriber2)
  subscriber ||= subscriber2
end

I think we can recognize old API vs new API based on event_types if it responds to each (whether that's first or second argument). Let's hope not many handlers respond to each and this best-effort heuristic be good enough_

@paneq

This comment has been minimized.

Show comment
Hide comment
@paneq

paneq Feb 21, 2018

Member

How does it compare to AS::Notifications naming?

Hmmm, good question

ActiveSupport::Notifications.instrument('render', extra: :information) do
  render plain: 'Foo'
end

ActiveSupport::Notifications.subscribe('render') do |name, start, finish, id, payload|
  name    # => String, name of the event (such as 'render' from above)
  start   # => Time, when the instrumented block started execution
  finish  # => Time, when the instrumented block ended execution
  id      # => String, unique ID for this notification
  payload # => Hash, the payload
end

BTW: Temporary Subscriptions

You can subscribe to some event temporarily while some block runs. For example, in

callback = lambda {|*args| ... }
ActiveSupport::Notifications.subscribed(callback, "sql.active_record") do
  ...
end

the callback will be called for all “sql.active_record” events instrumented during the execution of the block. The callback is unsubscribed automatically after that.

from: http://api.rubyonrails.org/classes/ActiveSupport/Notifications.html

I prefer my within proposal ;)

Member

paneq commented Feb 21, 2018

How does it compare to AS::Notifications naming?

Hmmm, good question

ActiveSupport::Notifications.instrument('render', extra: :information) do
  render plain: 'Foo'
end

ActiveSupport::Notifications.subscribe('render') do |name, start, finish, id, payload|
  name    # => String, name of the event (such as 'render' from above)
  start   # => Time, when the instrumented block started execution
  finish  # => Time, when the instrumented block ended execution
  id      # => String, unique ID for this notification
  payload # => Hash, the payload
end

BTW: Temporary Subscriptions

You can subscribe to some event temporarily while some block runs. For example, in

callback = lambda {|*args| ... }
ActiveSupport::Notifications.subscribed(callback, "sql.active_record") do
  ...
end

the callback will be called for all “sql.active_record” events instrumented during the execution of the block. The callback is unsubscribed automatically after that.

from: http://api.rubyonrails.org/classes/ActiveSupport/Notifications.html

I prefer my within proposal ;)

@paneq

This comment has been minimized.

Show comment
Hide comment
@paneq

paneq Feb 21, 2018

Member

I was not sure why in the original issue I proposed execute... But maybe to allow chaining multiple subscriptions.

event_store.within do
  cancel_order.call(order_id, user_id)
end.subscribe(OrderCancelled) do |event|
  head :ok
end.subscribe(OrderCancelled) do |event|
  render json: {error: "doh"}
end.execute

Alternatives: run, call

Member

paneq commented Feb 21, 2018

I was not sure why in the original issue I proposed execute... But maybe to allow chaining multiple subscriptions.

event_store.within do
  cancel_order.call(order_id, user_id)
end.subscribe(OrderCancelled) do |event|
  head :ok
end.subscribe(OrderCancelled) do |event|
  render json: {error: "doh"}
end.execute

Alternatives: run, call

@pawelpacana

This comment has been minimized.

Show comment
Hide comment
@pawelpacana

pawelpacana Feb 21, 2018

Member

Alternatives: run, call

call will always be idiomatic in Ruby

Member

pawelpacana commented Feb 21, 2018

Alternatives: run, call

call will always be idiomatic in Ruby

paneq added a commit that referenced this issue Feb 22, 2018

PoC - new and safe dynamic subscriptions API
Proof of concept

Before calling within current event_broker is deeply cloned and put on
top of thread-scoped stack. Within the #within() method subscription
manipulation operates on that copy. When the block finishes we bring
back the old object.

I believe (to be tested) this should be safe in case multiple threads
(puma etc) doing multiple #within operations simulataniously.

Assumption: It's ok that other threads manipulations to subscribers are
not visible when the thread is inside #within block.

Thread 1: [Broker A]
Thread 2: [Broker A, Broker A+B]
Thread 3: [Broker A, Broker A+C, Broker A+C+D]

Initially I wanted to add

* add_temporary_subscriber
* add_temporary_global_subscriber
(maybe thread_local instead of temporary would be a better name)
and then maybe refactor to a single method with some modifiers (global
or limited to some events, thread_local or not).

to Broker and make Broker handle those subscriptions in
Concurrent::ThreadLocalVar but I realized that maybe keeping it
thread-unware could be a better solution. Now it only knows how to
duplicate itself so that the copy can be enriched with temporary
subscriptions.

Is this cool or crazy?

Refs: #196 #155

paneq added a commit that referenced this issue Feb 23, 2018

paneq added a commit that referenced this issue Feb 23, 2018

paneq added a commit that referenced this issue Feb 25, 2018

Deprecate subscribe_to_all_events(subscriber, &within) API
Provides fallback and upgrade instructions. Older API becomes
thread-safe by calling newer API.

Issues: #155 #196 #197

paneq added a commit that referenced this issue Feb 25, 2018

Deprecate old subscribe API
For a moment without tests or mutation tests

Issues: #155 #196

paneq added a commit that referenced this issue Feb 25, 2018

@paneq paneq changed the title from Better API for dynamic subscribers which are procs to Better API for subscribers (dynamic and static) which are procs Feb 25, 2018

paneq added a commit that referenced this issue Feb 25, 2018

paneq added a commit that referenced this issue Feb 25, 2018

@paneq

This comment has been minimized.

Show comment
Hide comment
@paneq

paneq Feb 25, 2018

Member

It's baking. Soon

Member

paneq commented Feb 25, 2018

It's baking. Soon

paneq added a commit that referenced this issue Feb 26, 2018

paneq added a commit that referenced this issue Feb 26, 2018

Test incorrect invokactions
which does not adhere to v1 or v2 of the subscription API.

Issues: #155 #196

paneq added a commit that referenced this issue Feb 27, 2018

paneq added a commit that referenced this issue Feb 27, 2018

@paneq paneq self-assigned this Feb 27, 2018

@paneq paneq closed this Feb 27, 2018

@paneq paneq referenced this issue Feb 27, 2018

Closed

Documentation: Better API for subscribers #200

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment