jmettraux / ruote

a ruby workflow engine (dead)
MIT License
496 stars 74 forks source link

Nested forget after rewind #43

Closed chrisconley closed 12 years ago

chrisconley commented 12 years ago

Hi John,

Gotta another curveball for you. I haven't been able to get the test to display the issue I'm having correctly but hopefully there's enough here to give you an idea of what's going on.

Basically, I have "sub2" that I always want to forget and "possibly_start_over" which is a participant that determines if the cursor should be rewinded. In my production workflow, when "possibly_start_over" issues the "rewind" command, the workflow does not actually forget "sub2" - it still waits for it. But when I change the command to "reset", the workflow proceeds to "charly" as expected, forgetting "sub2".

Any ideas? Or can I give you more information?

Thanks, Chris

  def test_nested_forget_after_rewind
    pdef = Ruote.process_definition do
      define "sub1" do
        cursor do
          apply
          sub2 :forget => true
          bravo
        end
      end

      define "sub2" do
        cursor do
          alpha
        end
      end

      cursor :tag => "main" do
        sub1
        possibly_start_over
      end

      charly
    end

    alpha = @dashboard.register_participant :alpha, Ruote::StorageParticipant
    bravo = @dashboard.register_participant :bravo, Ruote::StorageParticipant
    charly = @dashboard.register_participant :charly, Ruote::StorageParticipant
    possibly_start_over = @dashboard.register_participant :possibly_start_over, Ruote::StorageParticipant

    wfid = @dashboard.launch(pdef)

    wait_for(:bravo)
    wi = bravo.first
    @dashboard.reply(wi)

    wait_for(:possibly_start_over)
    wi = possibly_start_over.first
    wi.fields["__command__"] = "rewind" # Will still wait for sub2
    # wi.fields["__command__"] = "reset" # Though resetting forgets sub2 as expected
    @dashboard.reply(wi)

    wait_for(:bravo)
    wi = bravo.first
    @dashboard.reply(wi)

    wait_for(:possibly_start_over)
    wi = possibly_start_over.first
    @dashboard.reply(wi)

    wait_for(:charly)
  end
jmettraux commented 12 years ago

I'll look at it after https://github.com/jmettraux/ruote/issues/42 but "forgotten" bits are not cancellable, I wonder if two distinct sub2 branches are not showing up.

@dashboard.noisy = true

will help us determining that.

Probably the test process definition can be made smaller, reduced to the essence of the issue (and if there are two issues, then let's have two distinct tests ;-) )

jmettraux commented 12 years ago

Hello Chris,

I found issues with the StorageParticipant (not saving workitems properly and mixing workitems from different participants), I will fix them and then report here. I think these two issues are impacting your case.

Stay tuned,

jmettraux commented 12 years ago

I only noticed now that you are using

@dashboard.reply(wi)

where you should be using

bravo.proceed(wi)
  # or
@dashboard.storage_participant.proceed(wi)

Calling #reply on the dashboard directly short-circuits the storage participants (the workitem stays in the storage) while the expression resumes. I will try to modify Dashboard#reply so that the participant properly receives the workitem, but I'm not sure I'll be successful, and when you get a workitem via a storage participant you have to hand it back to it via #update or #proceed.

I'm now trying to fix the issue where alpha and bravo are not isolated storage participant instances but a single storage participant (all the workitems show up, whatever the participant name).

chrisconley commented 12 years ago

Hi John,

Thanks very much for your efforts on this - it's much appreciated.

Also, thanks for the #proceed tip; that was what I needed to make a simpler test case. It sounds like you already have a plan but here is what I found in case it helps:

  def test_nested_forget_after_rewind
    pdef = Ruote.process_definition do
      bravo
      alpha :forget => true
      charly
    end

    alpha = @dashboard.register_participant :alpha, Ruote::StorageParticipant
    bravo = @dashboard.register_participant :bravo, Ruote::StorageParticipant
    charly = @dashboard.register_participant :charly, Ruote::StorageParticipant

    wfid = @dashboard.launch(pdef)

    wait_for(:bravo)
    wi = bravo.first
    bravo.proceed(wi)

    wait_for(:charly)
  end

The above test passes just fine. As will the next:

  def test_nested_forget_after_rewind
    pdef = Ruote.process_definition do
      cursor :tag => "main" do
        bravo
        alpha :forget => true
      end

      charly
    end

    alpha = @dashboard.register_participant :alpha, Ruote::StorageParticipant
    bravo = @dashboard.register_participant :bravo, Ruote::StorageParticipant
    charly = @dashboard.register_participant :charly, Ruote::StorageParticipant

    wfid = @dashboard.launch(pdef)

    wait_for(:bravo)
    wi = bravo.first
    bravo.proceed(wi)

    wait_for(:charly)
  end

But this will fail (hang):

  def test_nested_forget_after_rewind
    pdef = Ruote.process_definition do
      cursor :tag => "main" do
        bravo
        alpha :forget => true
        possibly_start_over
      end

      charly
    end

    alpha = @dashboard.register_participant :alpha, Ruote::StorageParticipant
    bravo = @dashboard.register_participant :bravo, Ruote::StorageParticipant
    charly = @dashboard.register_participant :charly, Ruote::StorageParticipant
    possibly_start_over = @dashboard.register_participant :possibly_start_over, Ruote::StorageParticipant

    wfid = @dashboard.launch(pdef)

    wait_for(:bravo)
    wi = bravo.first
    bravo.proceed(wi)

    wait_for(:possibly_start_over)
    wi = possibly_start_over.first
    wi.fields["__command__"] = "rewind"
    possibly_start_over.proceed(wi)

    puts "this will print"
    wait_for(:bravo)
    wi = bravo.first
    bravo.proceed(wi)

    puts "this won't print"
    wait_for(:possibly_start_over)
    wi = possibly_start_over.first
    possibly_start_over.proceed(wi)

    wait_for(:charly)
  end

Thanks again and let me know if I can help out in any way.

-Chris

jmettraux commented 12 years ago

Hello Chris,

you could replace

  wi = x.first

by

  wi = x.by_participant('x').first

I know it's ugly, but currently, bravo and possibly_start_over will return all the workitems in the storage, not just the workitem with their names. I'm working on fixing that...

Thanks a lot,

John

P.S.

In fact, you could write:

  def test_nested_forget_after_rewind
    pdef = Ruote.process_definition do
      bravo
      alpha :forget => true
      charly
    end

    @dashboard.register '.+', Ruote::StorageParticipant

    wfid = @dashboard.launch(pdef)

    wait_for(:bravo)
    wi = @dashboard.storage_participant.by_participant('bravo').first
    @dashboard.storage_participant.proceed(wi)

    wait_for(:charly)
  end
chrisconley commented 12 years ago

Thanks John, I wrote a little helper to shorten it up a bit:


  def test_nested_forget_after_rewind
    pdef = Ruote.process_definition do
      cursor :tag => "main" do
        bravo
        alpha :forget => true
        possibly_start_over
      end

      charly
    end

    @dashboard.register '.+', Ruote::StorageParticipant

    wfid = @dashboard.launch(pdef)

    wait_for_and_proceed(@dashboard, :bravo)
    wait_for_and_proceed(@dashboard, :possibly_start_over) {|wi|  wi.fields["__command__"] = "rewind" }

    wait_for_and_proceed(@dashboard, :bravo)
    wait_for_and_proceed(@dashboard, :possibly_start_over)

    wait_for(:charly)
  end

  def wait_for_and_proceed(dashboard, participant_name, &block)
    wait_for(participant_name)
    wi = dashboard.storage_participant.by_participant(participant_name.to_s).first
    yield wi if block_given?
    dashboard.storage_participant.proceed(wi)
  end

This test now passes, so is the only issue in the test implementation?

-Chris

jmettraux commented 12 years ago

Nice helper, I should add it to the Dashboard/WaitLogger.

I think the real issue is that the StorageParticipant doesn't behave "as expected" when doing

  a = @dashboard.register :a, Ruote::StorageParticipant
  wi = a.first

so yes. I need to fix that (will clean up Ruote::StorageParticipant). Suggestions and critics are welcome.

Thanks again,

chrisconley commented 12 years ago

Cool, thanks. I'll go ahead and close this issue out.

A method like #current_workitem seems most intuitive to me rather than #first. I'm not sure how this plays with a participant that has concurrent workitems though. Maybe #current_workitems would work better?

jmettraux commented 12 years ago

In fact, StorageParticipant includes Enumerable, so it sports #first, #each, #find, etc. I need to document that more visibly.