2

I'm working on a data import job using the Go language, I want to write each step as a closure, and use channels for communication, that is, each step is concurrent. The problem can be defined by the following structure.

  1. Get Widgets from data source
    1. Add translations from source 1 to Widgets.
    2. Add translations from source 2 to Widgets.
    3. Add pricing from source 1 to Widgets.
    4. Add WidgetRevisions to Widgets.
      1. Add translations from source 1 to WidgetRevisions
      2. Add translations from source 2 to WidgetRevisions

For the purposes of this question, I'm only dealing with the first three steps which must be taken on a new Widget. I assume on that basis that step four could be implemented as a pipeline step, which in itself is implemented in terms of a sub-three-step pipeline to control the *WidgetRevision*s

To that end I've been writing a small bit of code to give me the following API:

// A Pipeline is just a list of closures, and a smart 
// function to set them all off, keeping channels of
// communication between them.
p, e, d := NewPipeline()

// Add the three steps of the process
p.Add(whizWidgets)
p.Add(popWidgets)
p.Add(bangWidgets)

// Start putting things on the channel, kick off
// the pipeline, and drain the output channel
// (probably to disk, or a database somewhere)
go emit(e)
p.Execute()
drain(d)

I've implemented it already ( code at Gist or at the Go Playground) but it's deadlocking with a 100% success failure rate

The deadlock comes when calling p.Execute(), because presumably one of the channels is ending up with nothing to do, nothing being sent on any of them, and no work to do...

Adding a couple of lines of debug output to emit() and drain(), I see the following output, I believe the pipelining between the closure calls is correct, and I'm seeing some Widgets being omitted.

Emitting A Widget
Input Will Be Emitted On 0x420fdc80
Emitting A Widget
Emitting A Widget
Emitting A Widget
Output Will Drain From 0x420fdcd0
Pipeline reading from 0x420fdc80 writing to 0x420fdd20
Pipeline reading from 0x420fdd20 writing to 0x420fddc0
Pipeline reading from 0x420fddc0 writing to 0x42157000

Here's a few things I know about this approach:

  • I believe it's not uncommon for this design to "starve" one coroutine or another, I believe that's why this is deadlocking
  • I would prefer if the pipeline had things fed into it in the first place (API would implement Pipeline.Process(*Widget)
    • If I could make that work, the drain could be a "step" which just didn't pass anything on to the next function, that might be a cleaner API
  • I know I haven't implemented any kind of rung buffers, so it's entirely possible that I'll just overload the available memory of the machine
  • I don't really believe this is good Go style... but it seems to make use of a lot of Go features, but that isn't really a benefit
  • Because of the WidgetRevisions also needing a pipeline, I'd like to make my Pipeline more generic, maybe an interface{} type is the solution, I don't know Go well enough to determine if that'd be sensible or not yet.
  • I've been advised to consider implementing a mutex to guard against race conditions, but I believe I'm save as the closures will each operate on one particular unit of the Widget struct, however I'd be happy to be educated on that topic.

In Summary: How can I fix this code, should I fix this code, and if you were a more experienced go programmer than I, how would you solve this "sequential units of work" problem?

Lee Hambley
  • 6,270
  • 5
  • 49
  • 81
  • I have resolved the deadlocking question, the problem was two-fold, the `case i == len(pipe.processes)` was wrong, the `len()` is always `+1` (rookie mistake). – Lee Hambley Dec 10 '12 at 22:34
  • I also implemented that condition as `in, out = out, pipe.Drain` rather than making a new pipe, that was foolish... but the questions about this being a decent design stand. I feel like it has a potential to blow up in my face, and the types aren't reusable, although I am quite happy with the pipelining mechanism... – Lee Hambley Dec 10 '12 at 22:35
  • The final code, which is working is at http://play.golang.org/p/YXYLFR3qcz but it desperately needs to be made more generic. – Lee Hambley Dec 10 '12 at 22:36
  • The clue was that in every run the line `Output Will Drain From 0x420fdcd0` had the same value (including the address), but that address is not repeated in the pipeline i/o output. That lead me to look at the *end case* for the `Execute()` function, where I saw my mistake. – Lee Hambley Dec 10 '12 at 22:39
  • btw, using `var x = func()...` is kind of stage looking. Just use `func x()`. – Dustin Dec 11 '12 at 05:43
  • It seems desirable to specify the direction on channels (i.e. their "endedness") in programs like this because the compiler can eliminate common mistakes more easily. Here's my take on your example: http://play.golang.org/p/t6C0Av1jSI – Rick-777 Dec 11 '12 at 10:26

1 Answers1

2

I just don't think I would've built the abstractions that far away from the channels. Pipe explicitly.

You can pretty easily make a single function for all of the actual pipe manipulation, looking something like this:

type StageMangler func(*Widget)

func stage(f StageMangler, chi <-chan *Widget, cho chan<- *Widget) {
    for widget := range chi {
                f(widget)
                cho <- widget
    }
    close(cho)
}

Then you can pass in func(w *Widget) { w.Whiz = true} or similar to the stage builder.

Your add at that point could have a collection of these and their worker counts so a particular stage could have n workers a lot more easily.

I'm just not sure this is easier than piecing channels together directly unless you're building these pipelines at runtime.

Dustin
  • 89,080
  • 21
  • 111
  • 133
  • If you can add some usage example to this, then I'll gladly accept and upvote your answer, it seems more correct as the function definitions are simpler, and it doesn't rely on one-channel-per-object-per-stage plus-one as my implementation does. – Lee Hambley Dec 12 '12 at 18:53
  • There's nothing wrong with a channel per stage with each having a goroutine to do the processing. I don't know *exactly* how I'd implement the thing you're doing because I don't know *exactly* what your needs are. But having a `*Widget` output channel and instead of reading from that, having another thing (or group of things) read from that and output to another `*Widget` output channel in a pipeline feels very natural to an old UNIX programmer like me. – Dustin Dec 13 '12 at 04:51
  • I wanted to ask you you'd implement your method above, it seems like it would also need to do a lot of channel handling. Actually I want to parallelise a sequential workflow, that's the end goal, whiz, pop and bang all add (remotely queried) data to the widget in the real example, and executing them sequentially seems very wasteful. – Lee Hambley Dec 13 '12 at 07:51
  • I've implemented things like that. If you want them to happen concurrently, you need a way to update them concurrently, otherwise it's pretty simple. You do need to pipe the pointer into each channel in a loop, and probably have a WaitGroup on the widget to know when it's been fully processed. – Dustin Dec 14 '12 at 00:03