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

Ready callback #68

Open
Odaeus opened this issue Jan 14, 2021 · 4 comments · May be fixed by #76
Open

Ready callback #68

Odaeus opened this issue Jan 14, 2021 · 4 comments · May be fixed by #76

Comments

@Odaeus
Copy link
Contributor

Odaeus commented Jan 14, 2021

In addition to the handle_setup work I found I had trouble with failing tests due to timing issues between sending a message and the consumer being ready to receive. I see that Rabbit's own tests use polling to wait on the state of the consumer GenServer.

To improve this I've added an on_ready callback: Odaeus@b6abacd

I can't open a PR yet as it's built on top of #67. Please let me know if you'd be interested in incorporating it too. I'm not fixed on on_ready as a name but handle_ready didn't sound quite appropriate.

@Odaeus
Copy link
Contributor Author

Odaeus commented Mar 12, 2021

Please let me know if you have interest in a PR for this too @nsweeting.

@nsweeting
Copy link
Owner

@Odaeus Thanks for the suggestion.

I'm wondering if you could provide a bit more context on the use case you have where an outside process is needing to interact with a consumer process?

This might also be able to get solved in another way. If you look at Rabbit.Producer - you'll see that it provides a sync_start option -

| {:sync_start, boolean()}
. What this does it ensures that the producer is fully setup within the GenServer.init/1 callback. Within the context of a supervision tree - this means the producer will block until fully setup and any processes that rely on the producer can be assured it will be ready as long as they are placed after the producer within the tree.

We could provide this option within the consumer as well. I would typically default this sync_start option to false for a consumer since in my eyes - there are less use cases where an outside process is relying on a consumer vs an outside process relying on a producer.

@Odaeus
Copy link
Contributor Author

Odaeus commented Mar 12, 2021

My main use-case so far is so that integration tests can wait for the consumer to be ready before sending a message. I had a couple of other ideas about it being useful to notify other parts of the system that the consumer was ready. One was that it would notify another part of the system that it can now register the bindings for this queue, but I can achieve the same with our beloved handle_setup 😄 if I really need it.

I normally do my best to avoid adding code only for testing but I think anything is better than polling if it can be avoided. I think your sync_start solution seems pretty good as it's perfect for using with start_supervised. Happy for me to open a PR for that?

@nsweeting
Copy link
Owner

Yeah - I think adding sync_start would be the best solution for now as it mirrors other parts of the application as well. Absolutely open to PR's! Thanks!

@Odaeus Odaeus linked a pull request Mar 15, 2021 that will close this issue
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 a pull request may close this issue.

2 participants