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

Don't timeout a server connection while in a transaction. #399

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ximon18
Copy link
Member

@ximon18 ximon18 commented Oct 1, 2024

Extracted from the xfr branch.

The purpose of a transaction is to try and ensure that the whole set of responses (e.g. a zone transfer) are sent to the client, we don't want to send some but not all, so be more lenient with timing out connections during a transaction.

IIRC I added this after seeing issues in local testing with transfer of large zones.

As the purpose of a transaction is to try and ensure that the whole set of responses (e.g. a zone transfer) are sent to the client, we don't want to send some but not all and then timeout the other side for some reason.
@ximon18 ximon18 requested a review from a team October 1, 2024 10:56
@Philip-NLnetLabs
Copy link
Member

It would be best to include some measure of progress. In some cases a TCP connection can hang for 20 minutes if the other party disappears. That may be a bit much.

@ximon18
Copy link
Member Author

ximon18 commented Oct 3, 2024

Hi @Philip-NLnetLabs,

Thanks for the feedback, I think you make a good point about long hanging connections so there's definitely something to be done here, but I'm not quite sure exactly what you have in mind.

It would be best to include some measure of progress. In some cases a TCP connection can hang for 20 minutes if the other party disappears. That may be a bit much.

I'm not sure what you mean here by "include some measure of progress", neither "include" nor "measure of progress".

It occurs to me (and maybe you are also hinting at this?) that while there is an idle timeout (connection hasn't been read from or written to in X time) and a response write timeout (more than X time spent writing a single response) there is no timeout and maybe there should be one on an entire transaction (more than X time spent writing multiple responses that all belong to a single transaction).

I don't see how that relates to "measure of progress" or what it would mean to "include" such a measure. Are you referring perhaps to a metric to expose to the code owning the server instance?

@Philip-NLnetLabs
Copy link
Member

It boils down to defining a unit of work (for example sending 1 kB of data and the maximum time it is allowed to take. If it takes to long, abort the connection. The tricky thing in this case is dealing with bursts at the client side. The client may quickly consumes a lot of data and then needs to process it. Maybe a timer at the start, a counter for the total amount of data transferred and then compute when the next amount is due.

@partim
Copy link
Member

partim commented Dec 18, 2024

Is this something we want to as part of this PR or might it be better to just merge the PR and make an issue for later?

@ximon18
Copy link
Member Author

ximon18 commented Dec 18, 2024

As it fixes an actual problem I encountered I would vote for towards as you suggest @partim.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants