omniscale / imposm3

Imposm imports OpenStreetMap data into PostGIS
http://imposm.org/docs/imposm3/latest/
Apache License 2.0
726 stars 159 forks source link

Correctly stop diff parsing when done. #45

Closed rubenv closed 9 years ago

olt commented 9 years ago

Somehow the tests are failing with this change. Can you give me some details what this should fix? See System Tests in the README if you want to run the tests on your own.

rubenv commented 9 years ago

Maybe I'm using the parser in a wrong way. I'm currently doing this:

    changes, err := parser.Parse(u.Filename)

    for {
        c, ok := <-changes
        if !ok {
            break
        }

        err := u.process(c)
        if err != nil {
            return err
        }
    }

    return <-err

This should be the right way to do things, right?

Without this change, none of the channels ever get closed, which prevents this method from returning.

olt commented 9 years ago

Ok, you are using the parser in your own code. The err from Parse is a channel and can return an error anytime. You need to read from both channels with a select, see https://github.com/omniscale/imposm3/blob/master/diff/process.go#L142

But I'm seeing that checking for the osmChange EndElement is a good idea. Otherwise it might accept a truncated diff file (a bit unlikely since the gzip reader should fail for an incomplete stream).

I'll fix that. The select on changes and err is still required though.

PS: Be aware that imposm3/diff/parser is not a public/stable API and that it might change.

rubenv commented 9 years ago

PS: Be aware that imposm3/diff/parser is not a public/stable API and that it might change.

That's fine, I'm fully prepared to fix any breakage.

I'll publish my work soon (it's a topology extraction tool) but in the meantime I already want to send many thanks for imposm3: much of your code has really saved me a ton of work.