fix bug: #siphon - should pass the faultPipe

[osher]

When an error is thrown in any of the handlers - the flow is broken: I don't get to the last fitting, and I don't get to the fault-pipe handler.
I assumed we need to pass the faultPipe handler.

I created this branch online, then cloned it and went to add tests.
Then I saw there are more internal parts that are ignored...

Is it on purpose that it does not pass Pipeworks#pre and Pipeworks#post?

What if the user puts his end-handler with {affinity: sink} ?
we'll get broken the same way...

What if the user tries to measure exec-time using {affinity: hoist} ?

Any comment will be appreciated...

[osher]

I propose to add Pipeworks#clone(), and that Pipeworks#siphon should use that instead of creating the separated pipeworks "manually".

#clone - will create a cloned instance, keeping integrity with all the internal parts.

I did not read all the module, but I get a feeling it will be useful in other places...

[osher]

let me know how you want to go with it
If you like this proposal - I propose to do it incrementally in a separate PR - I need this change for a start

[osher]

follow up ;)

[kevinswiber]

Hi, @osher! I appreciate your persistence here.

Pipeworks#siphon is intended to redirect the flow to a new pipeline. Sequence and affinity are both important. If there is no fault pipe associated with the siphoned-to pipeline, then no fault pipe will be executed. This looks like intended behavior to me. If I'm missing something, I'm happy to be set straight. :) Let me know.

[osher]

Edit: fix some typos, some grammer + beautify

This is puzzling.
I have to make sure I understand correctly.

Assuming a pipe:

   pre:
      - handler1
   pipe:
      - handler 2
      - handler 3
      - handler 4
   post:
      - handler 5
   fault:
      - handle_err

You mean that if I .sihpon(..) it I effectively get only handlers 2~4?
You mean that .sihpon(..) means to execute handlers 2~4 within the pre, post and fault of the target pipe?

This feels inconsistent with the idea expressed by the api .fit({ affinity }, handler).
The way I understand this API is that this is how I get to construct my pipe, where the affinity is a tool to help me control order of execution by dividing it to general stages, where fittings can be appended dynamically to each of them.

Now true, .fault(..) is another api - which does raises the question why in fact it's not offered as fit({affinity:'fault'},..) , or on the other hand why the other two affinities are not implemented using their own methods - but all in all - it looked to me like the tools one gets to construct the pipe.

In that sense - I would expect the pre and post handlers to pass through, as well as the error handler.

Assume the example from the readme.
The one that outputs

// getting started
// am i getting hijacked?
// hijacked!
// done-zo

Now consider that instead of console.log('hijacked!'), the pipe was ending with an error.
Assume that the main pipe is the execution of a web-request.
In this case, the main pipe and the web-request - _stay hanged_, because nothing reports the error. It is effectively suppressed. It happened to me, and that's how I got to propose this PR.

It could still be my lack of understanding.

Here is the concrete case: I was trying to add supporting done callback to API implemented using pipeworks, I would appreciate your insight about it:

apigee-127/bagpipes@4861035

If we're not on target there, if the tool is misused or abused - I think people need to hear about it so they can do better

[osher]

Kevin - hi, here's a follow up - I need your input...

[osher]

> osher instanceof Persistent
true
> while( !osher.getReplyFrom( kevin ) ) ;
_

[kevinswiber]

Hi, @osher. Fault Pipes are only used in the context of their pipeline. It sounds like what you're looking for is a layer above a pipeline to manage multiple pipelines. There's nothing that prevents a user from setting newPipeline.faultPipe = oldPipeline.faultPipe before calling siphon. I'd be uncomfortable setting this as the default behavior. There may be code that depends on the existing behavior. A siphon call redirects the pipeline context completely, and it may never return. A siphon call can even happen in the pre flow. Moving over all pre, post, and fault pipes seems error prone. Is there a way to accomplish this in bagpipes while keeping the existing functionality in pipeworks? /cc @theganyo