Skip to content
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

WIP: add async client #62

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

Conversation

withinboredom
Copy link

@withinboredom withinboredom commented Feb 11, 2024

This PR adds an async client (using amphp). First, take a look at the performance:

mode pubs per sec recv per sec
async (background) 260,491 52,608
async (foreground) 260,888 101,162
sync 291,681 241,338

Async takes a HUGE performance hit, but it allows you to do other things while things are processing. So, this is a trade-off: less throughput for more flexibility.

Using it:

// this should be almost exactly the same
$client = new AsyncClient($config);
$client->subscribe('hello', $callback);

$stop = $client->background(enableAutoReply: true, concurrency: 10);

try {
  // or whatever you want to do here, but you need to yield to the event loop
  // to run the client, or do other async things.
  $bucket = $client->api->getBucket('bucket');
  $entry = $bucket->getEntry('key');

  // but if you don't have anything left to do, you MUST yield to the event loop at some point.
  EventLoop::run();
} finally {
  $stop();
}

// or you can process as usual
$client->process();

This is still a work in progress, but I'd love to know if you are interested in merging this.

@nekufa
Copy link
Member

nekufa commented Feb 12, 2024

Hi @withinboredom , looks interesting!
I thinks this one should be merged too, so anybody can choose runtime - sync/async.
Async is not related only with amphp, so maybe it's better to name AmpClient, for example?
In addition, it will be good to follow phpcs.xml convention, so whole library looks better..

@withinboredom
Copy link
Author

maybe it's better to name AmpClient

Good idea, thanks!

it will be good to follow phpcs.xml convention, so whole library looks better..

I'll have to figure out how to enable that!

@nekufa
Copy link
Member

nekufa commented Feb 14, 2024

@withinboredom ping should be sent when there were no activity on a socket, sync client implementation is sending ping when there are no updates and time interval is passed

@withinboredom
Copy link
Author

Ah. Thanks for noticing that! I was trying to work out what the logic was doing there! That makes it make so much sense now, perfect!

@withinboredom
Copy link
Author

Added a phpbench and report:

image

@withinboredom
Copy link
Author

There are still some issues, particularly around streams/consumers. I'm trying to figure out the best way to handle that.

@nekufa
Copy link
Member

nekufa commented Feb 20, 2024

@withinboredom looks like you miss async flag pass in process method for onMessage call

@nekufa
Copy link
Member

nekufa commented Feb 20, 2024

@withinboredom some comments:

  • composer.json should not require amphp packages, suggest should be enough
  • latest release exposes handler result, so you can replace break with return for msg type

@nekufa
Copy link
Member

nekufa commented Feb 21, 2024

i found an issue with consumer expires exceed client timeout, this way ping msg reset processing flags. rebase you changes, maybe it should solve some issues. also client message processing was split like your implementation

@withinboredom
Copy link
Author

rebase you changes

Nice! I'm on a holiday this week, but I'll be back at it this weekend. Looking forward to seeing the new stuff!

Last week, I was comparing the consumers here to the Go consumers and noticed idle heartbeats were missing, as well as naks, which I was planning to use for delayed messages -- basically, respond with a nak + delay header and it will be (re)delivered at a later time. The python library does this, so it would be great to support that as well, or at least make it easy for devs to implement themselves if they want to.

I have a bunch of notes for how the consumers could be improved but was going to implement them in my application code vs. here (as it would be a big breaking change of the consumer API). I can put them in a gist (or a PR here to a docs folder or something) to discuss them, if you'd be interested. Alternatively, I could put the code into a separate library that people could choose to use via a setter in this library (something like $client->setApi($otherApi) or something).

@nekufa
Copy link
Member

nekufa commented Feb 22, 2024

In addition, we can't use current consumer api for batch processing approach. I mean pass multiple set of params request to a handler.

If i can help with design or implementation, let me know. I think we can merge this request when all tests would be passed, release it in current major release and then analyze other language implementations and redesign php client.

Never mind on api breaking changes, semver supports breaking changes, so we can rethink anything and upgrade major release. I think that's okay.

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.

None yet

2 participants