Build failure on ARM

[mihaitodor]

Hey @twmb, thanks for this awesome package! I noticed that commit 31f3f5f changed p.waitBuffer = make(chan struct{}, 32) to p.waitBuffer = make(chan struct{}, math.MaxInt64) here, which seems to upset the compiler on 32-bit platforms. I noticed this after franz-go was updated to v1.8.0 in Benthos here.

[twmb]

Interesting -- the sync/atomic notes at the bottom indicate that 64 bit atomic operations are not guaranteed to be aligned on ARM. I don't have access to a 32 bit system to check alignment of all the atomics currently used, but they're almost certainly not aligned.

Also looking at the Benthos source, I'm pretty sure its atomics are not aligned either, example:

• Three sync.Map fields used here: https://github.com/benthosdev/benthos/blob/f7dac91e4e568848b406142734dc0fa6bf45e6d6/internal/bundle/tracing/events.go#L29
• sync.Map ends in an int, which should be 32 bit aligned on a 32 bit system: https://cs.opensource.google/go/go/+/refs/tags/go1.19.2:src/sync/map.go;l=66
• Meaning the second sync.Map is not aligned and the atomic ops within sync.Map are likely invalid (specifically within the read atomic.Value

I also highly doubt that the many many imported modules are all aligning atomics properly.

I'll see about setting up a 32 bit go vet with github actions, but it might be worth dropping arm support from Benthos' goreleaser.

I'll also change the make there to use math.MaxInt32 ... I'm pretty sure that should be safe, I'll stare at the code again. This is probably not important enough for a patch release, so I'd like to wait ~1day for a few other features and roll this into v1.9. I wasn't expecting to add a few things so quickly after v1.8, but we're here now.

[twmb]

In the above comment, I realize that the three sync.Map field internals are aligned, because I forgot the latter half of the sync/atomic note while writing: the first field of any struct is guaranteed to be 64 bit aligned.

So, benthos is probably fine. Not sure about the dependencies, though.

[mihaitodor]

Thank you so much for the quick investigation and the insight! Indeed, unless such issues are caught by the compiler, then there is a large surface to run into trouble at runtime... Ideally, people who need to run Benthos on a lightweight system will compile their own binary, importing only the packages they need instead of pulling in everything.

[twmb]

This will be fixed by #219, and the new github actions also will ensure this continues to work on arm -- including proper atomic alignment.

If you know actions better than I do (almost certainly) and know how to fix this chunk of broken code, that'd be super cool 😅

This will be out with 1.9 soon (this week hopefully, hopefully sooner)

[mihaitodor]

That's awesome, thank you so much for doing all this work so fast Travis! Ash opened an issue to check who is still interested in keeping these ARM builds before making any decision about deprecation, but, until then, this update should get the builds to pass.

For the CI, I see some examples in other repos, but I'll have to look closer at your setup. One thing that might be needed is a health check, to make sure those services are up and running (not just the container started), but Zookeeper and Kafka make that really difficult. However, if you'd consider replacing them with a Redpanda container, then that one has a HTTP endpoint which can be healthchecked easier if need be and it's also way faster to start up.

[twmb]

The integration tests previously didn't pass against redpanda due to some incompleteness in redpanda's implementation of transactions, but it looks like the tests now reliably pass (I know a lot of work has gone into improving transactions for the past few months, pretty cool to see it pay off). I'm quite keen to use redpanda in the integration tests.

[twmb]

This has been fixed and released in v1.9