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

progress: prevent resetting t_starttransfer #1616

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@rylwin
Contributor

rylwin commented Jun 26, 2017

Prevent Curl_pgrsTime from modifying t_starttransfer when invoked
with TIMER_STARTTRANSFER more than once during a single request.

When a redirect occurs, this is considered a new request and
t_starttransfer can be updated to reflect the t_starttransfer time
of the redirect request.

progress: prevent resetting t_starttransfer
Prevent `Curl_pgrsTime` from modifying `t_starttransfer` when invoked
with `TIMER_STARTTRANSFER` more than once during a single request.

When a redirect occurs, this is considered a new request and
`t_starttransfer` can be updated to reflect the `t_starttransfer` time
of the redirect request.
@mention-bot

This comment has been minimized.

mention-bot commented Jun 26, 2017

@rylwin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @yangtse and @captain-caveman2k to be potential reviewers.

@rylwin

This comment has been minimized.

Contributor

rylwin commented Jun 26, 2017

This change addresses the issue uncovered during the review of #1602

@rylwin rylwin force-pushed the rylwin:fix-repeated-starttransfer-progress branch 4 times, most recently to 42c59f2 Jun 26, 2017

@coveralls

This comment has been minimized.

coveralls commented Jun 26, 2017

Coverage Status

Coverage increased (+0.005%) to 73.816% when pulling 42c59f2 on rylwin:fix-repeated-starttransfer-progress into 922f800 on curl:master.

@bagder bagder closed this in f8f040e Jun 30, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jun 30, 2017

Thanks!

@dfandrich

This comment has been minimized.

Collaborator

dfandrich commented Jul 1, 2017

Since this was committed yesterday, about half my autobuilds (as well as others) fail test 1399 with "unit1399.c:95 Assertion 'usec_matches_seconds(data.progress.t_starttransfer, 3)' failed: about 3 second should have passed"

@bagder

This comment has been minimized.

Member

bagder commented Jul 1, 2017

We should probably first start with outputting what it thinks it got instead so that we understand better what the error is, as the function doesn't round.

@bagder bagder reopened this Jul 1, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jul 1, 2017

I suggest:

diff --git a/tests/unit/unit1399.c b/tests/unit/unit1399.c
index 1befc8aaf..b733c8fd8 100644
--- a/tests/unit/unit1399.c
+++ b/tests/unit/unit1399.c
@@ -37,11 +37,15 @@ static void unit_stop(void)
 }
 
 static bool usec_matches_seconds(time_t time_usec, int expected_seconds)
 {
   int time_sec = (int)(time_usec / usec_magnitude);
-  return time_sec == expected_seconds;
+  bool same = (time_sec == expected_seconds);
+  fprintf(stderr, "is %d us same as %d seconds? %s\n",
+          (int)time_usec, expected_seconds,
+          same?"Yes":"No");
+  return same;
 }
 
 UNITTEST_START
   struct Curl_easy data;
   struct timeval now = Curl_tvnow();
@coveralls

This comment has been minimized.

coveralls commented Jul 1, 2017

Coverage Status

Coverage increased (+0.2%) to 74.019% when pulling 42c59f2 on rylwin:fix-repeated-starttransfer-progress into 922f800 on curl:master.

bagder added a commit that referenced this pull request Jul 1, 2017

unit1399: add logging to time comparison
... to enable tracking down why autobuilds fail on this

Bug: #1616
@bagder

This comment has been minimized.

Member

bagder commented Jul 1, 2017

I cherry-picked that commit and merged it in 8d2b1de as this branch has a merge conflict. We can probably handle any upcoming additional bugfix as a new PR instead.

@rylwin

This comment has been minimized.

Contributor

rylwin commented Jul 1, 2017

OK, sounds good.

@dfandrich let me know if it would be helpful for me to look into the issues related to the changes here. I'd be happy to, I'm just not sure where to start as I don't see the failed builds on https://travis-ci.org/curl/curl/builds.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 1, 2017

@rylwin

This comment has been minimized.

Contributor

rylwin commented Jul 1, 2017

Thanks!

@bagder

This comment has been minimized.

Member

bagder commented Jul 2, 2017

Here's a clear example failure: https://curl.haxx.se/dev/log.cgi?id=20170702173244-21106#prob1 (link will only be alive for a few weeks)

@bagder

This comment has been minimized.

Member

bagder commented Jul 2, 2017

It is a 32 bit integer overflow in the test case on unit1399.c:69. We can't multiple epoch seconds with 1000000 on 32bit systems.

@rylwin

This comment has been minimized.

Contributor

rylwin commented Jul 2, 2017

Yep, I see it now. Thank you for pointing that out. I'll modify the test to avoid the issue and open a new PR for review.

rylwin added a commit to rylwin/curl that referenced this pull request Jul 2, 2017

bagder added a commit that referenced this pull request Jul 3, 2017

@bagder bagder closed this Jul 3, 2017

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