TS-4328 #554
TS-4328 #554
Conversation
…as succeeeded and data has been sent" This reverts commit 440a145. Conflicts: proxy/http/HttpTransact.cc
Before we would still retry if the origin didn't error-- but just took a really long time. So, instead of only checking if the body was sent-- we can check the milestone to see if we sent the request. If any bytes were sent the request cannot be retried. This closes apache#554
Before we would still retry if the origin didn't error-- but just took a really long time. So, instead of only checking if the body was sent-- we can check the milestone to see if we sent the request. If any bytes were sent the request cannot be retried. This closes apache#554
|
I did the original fix for this bug and this missed case definitely makes sense. I'm 👍 with the fix. The only thing that looks a little weird is using milestones as a flag but I suppose that's probably better than adding an unnecessary boolean flag. |
|
I looked at this for a while and I'm not convinced that it doesn't have unindented side-effects. The use of milestones is fairly unfortunate as well; I'd really like to avoid that. |
Before we would still retry if the origin didn't error-- but just took a really long time. So, instead of only checking if the body was sent-- we can check the milestone to see if we sent the request. If any bytes were sent the request cannot be retried. This closes apache#554
Before we would still retry if the origin didn't error-- but just took a really long time. So, instead of only checking if the body was sent-- we can check the milestone to see if we sent the request. If any bytes were sent the request cannot be retried. This closes apache#554 This also deprecates the `request_body_start` field, and as such removes it
|
After chatting with @jpeach we agree on the goal here (to not retry if bytes were sent on the wire). I have updated the PR to include comments attempting to explain the situation without the 10m chat we had on IRC ;) In addition since this effectively deprecates |
|
As per IRC discussion with @zwoop and @bryancall, I think we should find a way to not use the milestones to track the state transition. |
|
Can you guys expand on the concern with using milestones for this? I can easily add another bool to the state machine (or change the |
|
It's unfortunate because it couples internal state in the SM to the milestones, which are timing events. This means, changes to the milestones could have bad effects in other code, which would be really tricky to debug. It's sort of a violation of encapsulation, or even depending on side effects as a feature. |
|
Well, I'm not sure that I agree it violates the encapsulation-- since its effectively a variable that tracks timings (which are only set in specific states in the state machine-- IIRC most are only set once) which makes it more or less a history of where the HTTPSM has gone. For this particular feature I need to know "have we sent bytes" and today the only way that is stored is in the milestones. It sounds like both of you guys ( @zwoop and @jpeach ) want me to add a bool to the state machine to store this-- which I can do, but since the milestone is only set in one place, it'll effectively be the same check-- so i still don't really get how that would be "cleaner" in any way :/ |
|
The problem is that you are trying to "mix" the internals of a completely independent/different feature to use for an unrelated/unintended purposes. This sets a bad precedent and makes the code harder to maintain. You are looking at one specific example where it seems to be conveniently doing what you need "right now" - but, just bcoz it seems to be satisfying the needs right now doesn't mean it will continue to do so in the future, since milestones is a different feature/function intended to do completely different things. If tomorrow someone changes milestone implementation which is not entirely unlikely, it affects the rest of the code that depends on it so tightly because of its usage as "more" than milestones. Again, you might argue that when changing milestones one could also go ahead and change the rest of the code that "depends" on it. But, you now probably see the issue there? Think of people following the precedent set and start to randomly use milestones all over the place for various unrelated purposes. Milestones feature is no longer doing just milestones :) |
|
Oh, in any case, I haven't really checked carefully, but I actually wonder if the milestone (server begin write) in question here is reset for "each" server leg (assuming there's a 3xx redirect follow or some sort of other mechanism that is triggered)? If (not) so, does that still work the way you intended? |
Before we would still retry if the origin didn't error-- but just took a really long time. So, instead of only checking if the body was sent-- we can check the milestone to see if we sent the request. If any bytes were sent the request cannot be retried. This closes apache#554 This also deprecates the `request_body_start` field, and as such removes it
Before we would still retry if the origin didn't error-- but just took a really long time. So, instead of only checking if the body was sent-- we can check the SM to see if we wrote any header bytes to the wire. If any bytes were sent the request cannot be retried. This closes apache#554 This also deprecates the `request_body_start` field, and as such removes it
|
@zwoop @jpeach @SolidWallOfCode After chatting some more on IRC today, it seems the people are quite against the milestones approach-- so I've changed this patch to check the SM if we wrote header bytes -- which solves my immediate question "did I send bytes". I'm going to file a separate issue for some sort of API to see where the SM has been, since we don't want to use milestones for that. |
Before we would still retry if the origin didn't error-- but just took a really long time. So, instead of only checking if the body was sent-- we can check the SM to see if we wrote any header bytes to the wire. If any bytes were sent the request cannot be retried. This closes apache#554 This also deprecates the `request_body_start` field, and as such removes it
Before we would still retry if the origin didn't error-- but just took a really long time. So, instead of only checking if the body was sent-- we can check the SM to see if we wrote any header bytes to the wire. If any bytes were sent the request cannot be retried. This closes apache#554 This also deprecates the `request_body_start` field, and as such removes it
If the request hits a timeout-- but we sent the request to the origin we cannot rety the request
This test case was intended to test the case where the TCP session was established but no bytes were sent. The issue is that user space has no control over the SYN-ACK, accept() simply is user-space saying "i want this connection". So because of that this test is useless, and can be removed
Before we would still retry if the origin didn't error-- but just took a really long time. So, instead of only checking if the body was sent-- we can check the SM to see if we wrote any header bytes to the wire. If any bytes were sent the request cannot be retried. This closes #554 This also deprecates the `request_body_start` field, and as such removes it (cherry picked from commit f129996) Conflicts: proxy/http/HttpTransact.h
YTSATS-1558: Better python 2/3 compatibility for documentation.
…ures. (apache#8290) (apache#554) (cherry picked from commit 2e2bcd4) Conflicts: proxy/http/HttpTransact.cc
6 participants
Before we would still retry if the origin didn't error-- but just took a really long time. So, instead of only checking if the body was sent-- we can check the milestone to see if we sent the request. If any bytes were sent the request cannot be retried.