Don't kill a DnsExchangeBackground if a receiver is gone (see #1276)

[djc]

Fixes #1276

[codecov]

Codecov Report

Merging #1356 (4513d79) into main (ec8f839) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1356   +/-   ##
=======================================
  Coverage   86.29%   86.29%           
=======================================
  Files         132      132           
  Lines       13775    13775           
=======================================
  Hits        11886    11886           
  Misses       1889     1889           

[LEXUGE]

I am confused with this a little. Aren't receivers DnsExchangeBackground themselves? and DnsExchangeBackground should terminate itself when all clients are gone, otherwise it is meaningless. Maybe we shall come up with a approach to determine if all clients are gone. However, never terminating seems like not a good idea.

[djc]

It should still terminate, as I just explained in #1276. You're right that "receivers" is a bit confusing in this context. In this case I mean the DNS response futures which are waiting for a response from the background task.

[bluejekyll]

It's going to take me a moment to catch up with the root of this issue, could you summarize why this is correct?

Looking at this, it seems like there is a case where we had an error putting the response onto the receiver. What this will do, is assume that is a non-fatal error and keep the receiver open, and the overall exchanger will remain open for future requests.

I had to review the channel send docs:

If the value is successfully enqueued for the remote end to receive, then Ok(()) is returned. If the receiving end was dropped before this function was called, however, then Err(t) is returned.

So this is only the response channel that we're discussing, which would have been returned as part of the request submission. Given that the error is that the receiver end was closed, it implies incorrect usage of the result when the original request was sent. The request will be dropped at that point, meaning that it should never be sent (if we did our work properly and only perform work on poll). I think all of this makes sense, and in the context of queues being at play, it also makes to just drop even though the request will never be sent.

I think I'm ok with all of that, but I want to make sure we're on the same page in regards to the change of behavior this PR will have. @LEXUGE, by chance, have you been able to test with this change to see if it resolves, or improves, the behavior you were seeing in #1276?

[bluejekyll]

See comments in PR discussion, if we're all on the same page about this behavioral change, then feel free to merge.

[djc]

My take is that while we're in the process of forwarding a response future from the underlying connection to the requesting task, the receiver side of the channel (that is, the requesting future) has gone away. That is, we're now unable to forward the response future to the requesting task. There's (a) nothing we can do about this, and (b) it doesn't imply any kind of error that the background task shouldn't recover from. Hope that makes sense? Should I add more comments?

[LEXUGE]

Probably adding comments here to illustrate the situation could be better.

[djc]

Added, can you take a look and see if what I added makes sense?

[LEXUGE]

I can confirm this is effective towards #1276.

[bluejekyll]

Yes, @djc, I agree with all those points. I think @LEXUGE is correct that maybe adding a comment to that detail is probably worthwhile.

[bluejekyll]

I think that’s reasonable, @djc. Thank you for fixing this!