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

Fix a hanging ping call #109

Closed
wants to merge 1 commit into from
Closed

Fix a hanging ping call #109

wants to merge 1 commit into from

Conversation

mispon
Copy link

@mispon mispon commented May 22, 2023

See #91

@shueybubbles
Copy link
Collaborator

for tok := range tokChan {

does range tokChan not exit when len(t.tokChan) is zero?


Refers to: token.go:974 in 23ad6bd. [](commit_id = 23ad6bd, deletion_comment = False)

@mispon
Copy link
Author

mispon commented May 22, 2023

does range tokChan not exit when len(t.tokChan) is zero?

No, I check on the code snippet below. If we start the range-loop on the open empty channel, we just block it here

func main() {
	ch := make(chan struct{}, 5)
	for data := range ch {
		fmt.Println(data)
	}
	fmt.Println("done!")
}

@tiwo
Copy link

tiwo commented Oct 23, 2023

This is no solution; the range loop will come to an end once the channel is closed; I'm guessing this happens / should happen in ProcessSingleRepsonse in line 967

I think Ping should block under the circumstances of issue 91, and this seems to be the right place to block. see my comment on the issue.

@tiwo
Copy link

tiwo commented Oct 23, 2023

This PR tries to make this loop non-blocking.

(By the way, it does so the wrong way: By asking for the number of items waiting, and then accessing the channel. Hence the "RACE CONDITION" errors in the AppVeyor tests. For an example of distinguishing between "item received", "channel is closed" and "currently no item waiting", look above in the same file for syntax - but I'm not sure if I understand the comments there.

A relevant race condition can't happen here. Either way, this check doesn't help.)

After an abort signal to the server, this loop over range tokChan discards everything up until the server confirms the cancellation and is ready for the next action. This is necessary, as stray answers from the aborted operation could still come in, and need to be skipped.

It's allright if tokChan is temporarily exhausted - think slow network -, and then it's necessary to wait here. If the TCP connection finally fails, I assume tokChan would be closed by the producing side, maybe here in processSingleResponse.

@mispon
Copy link
Author

mispon commented Oct 24, 2023

I have no objection to that, so I will close this PR :)

@mispon mispon closed this Oct 24, 2023
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.

3 participants