loss detection timer description could be simplified by definining a timer mode

[marten-seemann]

In SetLossDetectionTimer we're setting the loss detection timer in order to:

• trigger an early retransmit (if we received gaps in the acknowledged packets) or
• trigger a PTO (if we only have a tail outstanding)

When the timer fires, in OnLossDetectionTimeout, we first need to figure out why we set the timer, and trigger an early retransmit or a PTO based on that.

I think the code would be easier if we save a "timer mode" enum { EARLY_RETRANSMIT, PTO } in SetLossDetectionTimer, and take action based on that mode in OnLossDetectionTimeout.

@ianswett, @janaiyengar What do you think?

[ianswett]

That could work, but I'd call it { LOSS_DETECTION, PTO }

Any interest in writing up a PR so we can see what the change you have in mind looks like?

[ianswett]

I wrote up #3182, but I don't think it's an improvement. Maybe it wasn't what you had in mind, @marten-seemann ?

[marten-seemann]

@ianswett I have a half-baked PR on my disk, but I still need to figure out the details of how this interacts the amplification limit. It seems like on the client side, there's a third timer mode (send anything to unblock the server from the 3x limit).
I'll put some more work into my PR and post it by early next week.

[ianswett]

One other idea would be to add a second timer, so you'd have a loss_timer and pto_timer. I was strongly against this when we had handshake retransmissions and TLP retransmissions and RTO retransmissions, but now that we're down to PTO, I like the idea better. I think that might be simpler to understand than an enum, and you could split the pseudocode into two methods.

[janaiyengar]

If we are going to make a change, I would very much argue for using a single timer. There's never a time when we might want both timers, which is signaled clearly to implementers by using a single timer. We can refactor the handling of timeouts though.

…

On Sat, Nov 2, 2019, 12:11 PM ianswett ***@***.***> wrote: One other idea would be to add a second timer, so you'd have a loss_timer and pto_timer. I was strongly against this when we had handshake retransmissions and TLP retransmissions and RTO retransmissions, but now that we're down to PTO, I like the idea better. I think that might be simpler to understand than an enum, and you could split the pseudocode into two methods. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3151?email_source=notifications&email_token=ACUOBVCZ4BN6YOF5ATH62PDQRXGFXA5CNFSM4JE5JFSKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC5C3LY#issuecomment-549072303>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUOBVGBOLNG37GJS2A6WIDQRXGFXANCNFSM4JE5JFSA> .

[ianswett]

@mnot All changes discussed are in the pseudocode, so I believe this will be editorial.

[larseggert]

Changed per discussion in ZRH

[ianswett]

I don't think we should do this anymore, and I have no intent to fix it.