pkgsite style docs for statemachine actor

[aol-nnov]

@JeroenSoeters please look what I've done. Moving things around that much was a bold move of mine, but I hope, you'll find grain of truth in the changes.

@halturin are you okay with godoc docs or any other format is preferred?

[JeroenSoeters]

Hi @aol-nnov, just acknowledging that I've seen this. I have some time later this week to look at this more carefully.

[aol-nnov]

@JeroenSoeters I've copied this wordings from your rfc, but I'm not sure if it is correct now as callbacks are actually returning state and data.

Also, this is my main concern, actually. If statemachine data is a complex struct, passing it between callbacks will be at cost.

[JeroenSoeters]

@aol-nnov apologies for the late reply on this, and thanks for putting the work in. The documentation is a welcome addition.

Here's myfeedback:

File split: splitting statemachine.go into separate files within the main package works for me.

Sub-packages (action/, cb/): I can't go with these unfortunately. They break backwards compatibility for all existing consumers. We have a plugin architecture where external devs write plugin for our tool (formae). Plugins currently import ergo.services/actor/statemachine and reference types like statemachine.Action, statemachine.StateMessageHandler directly. Moving these into sub-packages would require every consumer to update their imports. I'd like to keep the public API surface in the main package.

Timeout interface: this interface isn't referenced by anything in the PR. I'd prefer not to add abstractions without a concrete consumer.

Typos: statrmachine appears twice in the doc.go code example.

Cost of passing D by value: fair point in general, but in practice state changes typically are not in the hot path. I intentionally chose a more functional approach where handlers receive and return data rather than mutating shared state.

Once the sub-package changes are addressed, there will be conflicts with the recent changes on v310 (generation-based stale timeout detection, cancelled to canceled rename, diagnostic logging). Happy to help with the rebase at that point, just let me know.

[aol-nnov]

@JeroenSoeters glad you came finally, ha!

They break backwards compatibility

not sure what compatibility you're talking about while the whole component is not merged into master. Make it fair: it's a statemachine actor for other consumers (including formae), not the other way around.

So, I'm not convinced here...

changes typically are not in the hot path

Not quite understand, what do you mean by "typically". I've implemented a sip engine based on this statemachine and it spends most of the time in the transitions between states. See rfc3261 regarding transaction and dialog state machines, for example. It's gonna be a general purpose component, so "changes typically are not in the hot path in formae" does not automatically means "changes typically are not in the hot path in general".

Timeout interface

Yes, it leaked from newer changes I was going to share (actually, that were parts of a race fix you're implementing in another PR. "what a strike of luck"), I'll remove it from current changeset and update the typos. You're welcome to join any time as edits are allowed for maintainers in this PR.

[JeroenSoeters]

@JeroenSoeters glad you came finally, ha!

They break backwards compatibility

not sure what compatibility you're talking about while the whole component is not merged into master. Make it fair: it's a statemachine actor for other consumers (including formae), not the other way around.

So, I'm not convinced here...

changes typically are not in the hot path

Not quite understand, what do you mean by "typically". I've implemented a sip engine based on this statemachine and it spends most of the time in the transitions between states. See rfc3261 regarding transaction and dialog state machines, for example. It's gonna be a general purpose component, so "changes typically are not in the hot path in formae" does not automatically means "changes typically are not in the hot path in general".

Timeout interface

Yes, it leaked from newer changes I was going to share (actually, that were parts of a race fix you're implementing in another PR. "what a strike of luck"), I'll remove it from current changeset and update the typos. You're welcome to join any time as edits are allowed for maintainers in this PR.

@aol-nnov thanks for the pushback, let me address each point.

On backwards compatibility: my concern isn't just about formae. Looking at the other actors in this repository (saga, cqrs), they both keep all public types in a single package. The main ergo framework uses sub-packages (gen, act, lib) but within each sub-package all related types live together without further subdivision. The action/ and cb/ split would be the only case in the ergo ecosystem where an actor's callback types and action types live in separate sub-packages. I'd rather stay consistent with the existing conventions.

On the "not merged to master" point: while that's technically true, we do have real users today. formae has a plugin ecosystem where external developers build plugins that import this package. These plugins are in production use. Whether the code is on master or a release branch, the reality is that people depend on this API, and as the maintainer of this package I can't justify a breaking change to all existing consumers without a proportional benefit. Please keep this in mind for future contributions as well: API changes that break existing consumers need a strong justification.

On the hot path: fair point that formae's usage pattern shouldn't define the general case. But consider that the statemachine dispatches every message through reflect.Value.Call(), which allocates interface{} boxes for each argument and has a significant fixed overhead per invocation. Struct copying scales linearly with size but is essentially a memcpy at L1 cache speeds. For any reasonably sized data struct, the reflection overhead dominates the total cost by an order of magnitude. The copy cost only becomes comparable at multi-kilobyte struct sizes, and at that point reflection's own internal allocations are the bigger problem.

So if someone is building a high-throughput SIP engine where transitions are in the hot path, the reflection-based dispatch itself would be the first thing to address, not the value copying.

That said, there's already a simple escape hatch: instantiate with a pointer type. StateMachine[*MySIPData] means D is *MySIPData, so every "copy" just copies a pointer. The reflection dispatch handles pointer types correctly. No framework changes needed.

On the Timeout interface: thanks for removing it, makes sense.

[aol-nnov]

Oh, you're very welcome @JeroenSoeters and let me push it a bit more.

How do your users consume this package (and consider it stable!) if it even does not have a tag? I had to add it to my project by a commit id...

Don't get me wrong, I'm not that type of a crocodile every day, but the whole situation with this stale code laying around on a feature branch for years with no clear status, no docs or prompt communication from authors is really frustrating. And then out of blue there are millions of users tied to this public API that we "ain't goonna break with your silly changes". Why sharing at all if you're not open to changes? I'm afraid with your rhetorics, this parts of code will stay rigid and later become unmaintained legacy.

You know, large language models would chew through this thousand-line spaghetti with undisguised delight despite code splitting by files/packages (or lack of it), docs (or lack of them), etc, etc, but do not forget about real coders, the endangered species.

[aol-nnov]

@JeroenSoeters your concerns have been addressed.

[JeroenSoeters]

@aol-nnov the sub-packages (action/, cb/) are still there. The compat.go file re-exports the types as deprecated aliases, which means every consumer's IDE and linter will immediately flag warnings pushing them to migrate to the sub-package imports. That's not what I asked for. I asked to keep the types in the main package, not to deprecate them in favor of sub-packages.

Please move the types back into the main package. The file split within the main package and the documentation are welcome, the sub-packages are not.

[aol-nnov]

The compat.go file re-exports the types as deprecated aliases

You're absolutely right, that's what latest changeset reads. And I must sadly admit, you're adding requirements as we chat. Later you were concerned about backwards compatibility? Well, this one is now addressed and older consumers will successfully compile against this new code. Moreover the clear migration path is provided.

In golang packages are not only about structuring code, but also about structuring documentation. While you're tending to maintain chaos putting everything in a single package, I'm paving a way for future improvements in a more structured way.

Not sure if I could invest more time into this right now. At least I won't make you wait another month for the reply. Feel free to bend it as you see.

Best,
Andrey