Fix ctx propagated in NewPublisher #9
Conversation
|
I've a similar fix ready for the Subscriber. I would appreciate to get some feedback on the fix for the publisher before sending a pull request for the subscriber. |
| return nil, err | ||
|
|
||
| clientResultStream := make(chan clientResult) | ||
| go func() { |
There was a problem hiding this comment.
@rvs-fluid-it I'm wondering if it would be possible to hide all this complexity in a separate func? Perhaps so we could re-use the same logic in subscriber as well?
There was a problem hiding this comment.
Good point. But how do we move from here? Maybe it's better that I prepare a new pull request containing fixes for the publisher as well for the subscriber. And I will pay attention to minimize duplication ... What do you think? Do you have other points that I need to take in to consideration?
There was a problem hiding this comment.
It's fine to update this PR or create a new one, whatever works for you. 🙂 I think the idea overall is solid, I would just pay attention so that the Publisher and Subscriber constructors are easy to follow, as it gets quite complex with select and two contexts. However, hiding this complexity in a separate function should be enough.
There was a problem hiding this comment.
Hello, @rvs-fluid-it! I'm doing some housekeeping in Watermill repositories and I want to know if you have any update on that? 😉
If you don't have time now to work on that please let me know, so I can take care of that. Thanks!
The grpc connection which is opened when NewPublisher is called is closed too early.
When testing against the Google cloud pubsub emulator you can not reproduce the issue because it is opening the grpc connection differently. But when the Google cloud client connects to the real service you 'll hit the issue.