Skip to content
This repository has been archived by the owner on Aug 19, 2022. It is now read-only.

Don't wait for a chan that will never close #65

Merged
merged 1 commit into from
Jul 6, 2022

Conversation

MarcoPolo
Copy link
Contributor

Fixes a subtle bug where we assumed we would always have a background task when we have a trace object. This would cause trace.Close() to hang indefinitely.

The fix is to use a waitgroup instead of a semaphore channel. Then we add to the waitgroup when we spawn a background task.

Copy link

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

LGTM.

You could also create the channel at the point the where you add to the WaitGroup, and then in Close check if the channel is not nil before waiting on it.

@MarcoPolo
Copy link
Contributor Author

LGTM.

You could also create the channel at the point the where you add to the WaitGroup, and then in Close check if the channel is not nil before waiting on it.

Good to know! Are there any advantages compared to using a waitgroup?

My reasoning with the waitgroup is that if another future trace/reporter wants to do a background task they can do so easily by incrementing a waitgroup rather than managing multiple channels (or doing this refactor).

@MarcoPolo MarcoPolo merged commit 9b2aa9d into master Jul 6, 2022
@gammazero
Copy link

Are there any advantages compared to using a waitgroup?

It is more about conveying meaning, with the channel it clearly says you are waiting on one thing. I don't think it really matters here.

@ajnavarro ajnavarro mentioned this pull request Aug 24, 2022
72 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants