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

Revisit target provider implementations #1406

Closed
fabxc opened this Issue Feb 18, 2016 · 8 comments

Comments

Projects
None yet
6 participants
@fabxc
Copy link
Member

fabxc commented Feb 18, 2016

Various SD integrations are not using the given channels correctly. Most commonly they send on the update channel without listening on the done channel to abort in case the consumer is gone. This can cause deadlocking of single goroutines or the entire server.

Generally, the pattern of starting background processing before Run() is called should be reconsidered as well.

@cstyan

This comment has been minimized.

Copy link
Contributor

cstyan commented Aug 11, 2017

@fabxc I'm considering taking a shot at this, I think it would be a good task for improving my understanding of how the SD component works.

I could use some pointers as to where to start, and what the desired outcome is. Just to confirm I understand your first point, the done channel refers ctx.Done()?

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented Aug 12, 2017

I think I did a pass over the SDs some time ago and fixed some of the issues. But we added more SDs since so it's possible we do have some issues again.
There are certainly other general issues across various implementations like #3001

It's certainly a good idea to just read through them and try to validate their behavior! If you have any questions along the way, just ask here or on IRC.

The done channel refers to ctx.Done() as you said.

#3014 and #3013 are relevant cleanups that should how a low entry barrier as well. Those are probably best done against the dev-2.0 branch.

@cstyan

This comment has been minimized.

Copy link
Contributor

cstyan commented Aug 13, 2017

I'll look into the three issues you've linked,thanks!

@krasi-georgiev

This comment has been minimized.

Copy link
Member

krasi-georgiev commented Feb 17, 2018

@fabxc , @brancz , @gouthamve I think this can be closed now?

@brancz

This comment has been minimized.

Copy link
Member

brancz commented Feb 17, 2018

I would think so as well if anyone thinks otherwise feel free to re open.

@krasi-georgiev

This comment has been minimized.

Copy link
Member

krasi-georgiev commented Feb 17, 2018

@brancz i think you missed the close button :)

@brancz

This comment has been minimized.

Copy link
Member

brancz commented Feb 17, 2018

:facepalm:

@lock

This comment has been minimized.

Copy link

lock bot commented Mar 22, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 22, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.