Skip to content

feat(go/nats): support concurrent message handling #801

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

brooksmtownsend
Copy link
Contributor

This PR changes the Go NATS client's handleMessage function in a small way, using a goroutine to handle the message instead of in the current execution context.

This came in response from a bug request on the wasmCloud community side that multiple concurrent invocations weren't being handled in parallel, so I think this should resolve that issue. I added (had copilot write) two tests to ensure that this functionality is correct for handling multiple requests, and I did validate that without this fix the test fails.

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

simplify panic recovery

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>
Signed-off-by: Brooks Townsend <brooks@cosmonic.com>

simplify test

Signed-off-by: Brooks Townsend <brooks@cosmonic.com>
Copy link
Member

@rvolosatovs rvolosatovs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!
I'm concerned about a few things in this implementation:

  • unbounded goroutine count and corresponding memory/resource usage growth
  • panic handling - I am not convinced that simply logging the panic is sufficient for every use case
  • changed semantics regarding concurrency - currently there is no need for synchronization - this PR would change that and potentially break every user of this library
  • maintenance cost/inconsistent behavior - TCP transport does not do this, for example, so servers would behave differently depending on the transport, which is against the principles of this project. I don't think we want to be reimplementing this logic for each transport
  • (lower priority) perf cost - if an export does something very cheap, e.g. simply returns a constant - spawning a goroutine is redundant and likely would hurt the throughout, while also increasing memory usage

Personally, I don't think that wRPC should be blindly spawning goroutines - in my opinion it's the embeddeders job to do that in application-specific way.
That said, I'm not against introducing this functionality, but:

  • it must be generic, i.e. not be transport-specific
  • it must be configurable on per-export basis (just blanket on/off switch is fine for MVP, if path forward is clear)
  • embedder must be handling the panic
  • the goroutine count must be bounded in some way

Putting it all together I think the right place to put this feature would be the bindgen.

Therefore, here's what I think we should do:

  • by default spawn a goroutine for each function marked as async in WIT (imports and exports, but just exports for now)
  • reuse the async flag that already exists in the bindgen for configuring this functionality

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants