Fix ambiguous duration units and add format check by sunjayBhatia · Pull Request #12225 · envoyproxy/envoy

sunjayBhatia · 2020-07-22T16:28:23Z

Commit Message:
Fix ambiguous duration units and add format check

ambiguous value-based std::chrono::{clock_type}::duration(value) constructors
result in stdlib implementation specific default time units which are
hard to read and potentially different on different platforms
this change removes any instances of these ambiguous constructions and adds
a format check to prevent them; developers should specify an explicit unit of time
we explicitly saw this issue in router: add config to send date header with requests #11915 where
the assumed duration unit was different on Windows, causing test failures

Additional Description:
Risk Level: Low
Testing: Adds format check test and adjust existing unit tests
Docs Changes: N/A
Release Notes: N/A

- ambiguous value-based std::chrono::{clock_type}::duration(value) constructors result in stdlib implementation specific default time units which are hard to read - this change removes any instances of these ambiguous constructions and adds a format check to prevent them; developers should specify an explicit unit of time - we explicitly saw this issue in #11915 Co-authored-by: William A Rowe Jr <wrowe@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia · 2020-07-22T16:28:44Z

cc @wrowe @davinci26 @nigriMSFT

rebello95

I think this lint rule would be good to add 👍

test/extensions/filters/network/kafka/broker/filter_unit_test.cc

tools/code_format/check_format.py

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

zuercher · 2020-07-22T22:31:04Z

There's also this:

https://github.com/envoyproxy/envoy/blob/master/include/envoy/event/timer.h#L83

It doesn't look like TimeSystem::Duration is used anywhere, but it seems like it would have the same problems.

zuercher · 2020-07-22T22:31:31Z

@jmarantz you may have input on this as well.

- Ensure [Dd]uration is the complete method name - Allow for unambiguous values 0 or 0.0 Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

…hrono-duration Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

…hrono-duration Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

sunjayBhatia · 2020-07-24T21:14:08Z

Seems like the ARM job is generally unhealthy on master as well, otherwise i think this is ready?

zuercher

Looks good. I'll circle around and look at the build failures in a bit, but we'll probably need to merge master to pick up some fix before we can merge this.

…hrono-duration Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

zuercher · 2020-07-27T21:03:33Z

I think Josh is away, @lizan can you have a quick look at this?

lizan

This change looks good by itself. Though aren't we on a long term path toward absl::Time for all durations/time? Let's define a common duration in TimeSystem (there is already one there but ambiguous) without ambiguity and use them?

…hrono-duration Signed-off-by: wrowe@vmware.com

wrowe · 2020-07-28T16:23:49Z

Though aren't we on a long term path toward absl::Time for all durations/time? Let's define a common duration in TimeSystem (there is already one there but ambiguous) without ambiguity and use them?

The issue doesn't change, though. an absl::Duration(1000) construction would be equally ambiguous as the std::chrono classes currently in use, because absl also doesn't define the unit of measure, it instead provides explicit constructors based on the time precision desired (including expressing a duration as a ratio);

https://abseil.io/docs/cpp/guides/time#:~:text=An%20absl%3A%3ADuration%20represents,natural%20integer%2Dlike%20arithmetic%20operations.

It appears that the absl::Duration class doesn't even have a constructor to accept a plain int, so we wouldn't have needed to address that ambiguity, although our code check does note this occurrence whether the author notes the compilation error, or not.

It seems that the effort to convert this MonotonicTime Duration storage falls in the scope of a completely different PR. But as long as we are dealing with so many third party libraries, some of which use std::chrono classes, this particular PR seems like the right immediate approach and a long-term win.

lizan · 2020-07-28T21:34:05Z

Though aren't we on a long term path toward absl::Time for all durations/time? Let's define a common duration in TimeSystem (there is already one there but ambiguous) without ambiguity and use them?

The issue doesn't change, though. an absl::Duration(1000) construction would be equally ambiguous as the std::chrono classes currently in use, because absl also doesn't define the unit of measure, it instead provides explicit constructors based on the time precision desired (including expressing a duration as a ratio);

https://abseil.io/docs/cpp/guides/time#:~:text=An%20absl%3A%3ADuration%20represents,natural%20integer%2Dlike%20arithmetic%20operations.

It appears that the absl::Duration class doesn't even have a constructor to accept a plain int, so we wouldn't have needed to address that ambiguity, although our code check does note this occurrence whether the author notes the compilation error, or not.

It seems that the effort to convert this MonotonicTime Duration storage falls in the scope of a completely different PR. But as long as we are dealing with so many third party libraries, some of which use std::chrono classes, this particular PR seems like the right immediate approach and a long-term win.

What I'm saying is define a type alias in timesystem for further replacement. That was one of the point using MonotonicTime/SystemTime rather than leaking std::chrono everywhere. You can name it without ambiguity, e.g. 'using DurationMs = std::chrono::milliseconds`?

sunjayBhatia · 2020-07-29T15:05:43Z

What I'm saying is define a type alias in timesystem for further replacement. That was one of the point using MonotonicTime/SystemTime rather than leaking std::chrono everywhere. You can name it without ambiguity, e.g. 'using DurationMs = std::chrono::milliseconds`?

Do you want to replace TimeSystem::Duration with something or just add a new concrete duration alias?

Replacing sounds like it would break things like

envoy/test/test_common/test_time_system.h

Lines 18 to 48 in efd8ba9

    
             /** 
        
              * Advances time forward by the specified duration, running any timers 
        
              * scheduled to fire, and blocking until the timer callbacks are complete. 
        
              * See also advanceTimeAsync(), which does not block. 
        
              * 
        
              * This function should be used in multi-threaded tests, where other 
        
              * threads are running dispatcher loops. Integration tests should usually 
        
              * use this variant. 
        
              * 
        
              * @param duration The amount of time to sleep. 
        
              */ 
        
             virtual void advanceTimeWait(const Duration& duration) PURE; 
        
             template <class D> void advanceTimeWait(const D& duration) { 
        
               advanceTimeWait(std::chrono::duration_cast<Duration>(duration)); 
        
             } 
        
             /** 
        
              * Advances time forward by the specified duration. Timers may be triggered on 
        
              * their threads, but unlike advanceTimeWait(), this method does not block 
        
              * waiting for them to complete. 
        
              * 
        
              * This function should be used in single-threaded tests, in scenarios where 
        
              * after time is advanced, the main test thread will run a dispatcher 
        
              * loop. Unit tests will often use this variant. 
        
              * 
        
              * @param duration The amount of time to sleep. 
        
              */ 
        
             virtual void advanceTimeAsync(const Duration& duration) PURE; 
        
             template <class D> void advanceTimeAsync(const D& duration) { 
        
               advanceTimeAsync(std::chrono::duration_cast<Duration>(duration)); 
        
             }

e.g.

envoy/test/extensions/quic_listeners/quiche/platform/envoy_quic_clock_test.cc

Line 32 in efd8ba9

time_system.advanceTimeWait(std::chrono::microseconds(10));

e.g. https://godbolt.org/z/nKsq7s

the TimeSystem::Duration* type would likely have to be in nanoseconds to encompass all potential uses and all the tests that use the above methods would have to be converted to the equivalent of nanoseconds it seems This is wrong, implicit conversion does happen if a unit can be converted to another, that seems ok, but if a test or something uses a representation or ratio that cannot be converted this will still be an issue

It feels ok to have a generic TimeSystem::Duration type that does not refer to any specific unit to be used like the above, but not ok to try to create a duration using that type b/c it could be ambiguous (which the format check prevents)

lizan · 2020-07-30T04:08:01Z

I'm OK with either, replacing TimeSystem::Duration sounds good too. What I want to prevent is leaking std::chrono to more places.

…hrono-duration Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

- Makes it more trivial to switch to absl::Duration at some point Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com> Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

wrowe · 2020-07-30T19:10:39Z

What I want to prevent is leaking std::chrono to more places.

Let us know if the proposed change works for you?

sunjayBhatia · 2020-07-30T19:13:54Z

I'm OK with either, replacing TimeSystem::Duration sounds good too. What I want to prevent is leaking std::chrono to more places.

It would slow this patch down to do all the replacement of std::chrono duration types, so we will open an issue for that unless there are objections

(here's the issue: #12384)

lizan · 2020-07-30T21:16:08Z

I'm OK with either, replacing TimeSystem::Duration sounds good too. What I want to prevent is leaking std::chrono to more places.

It would slow this patch down to do all the replacement of std::chrono duration types, so we will open an issue for that unless there are objections

(here's the issue: #12384)

SGTM. We don't have to do all replacement in this PR.

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia · 2020-07-30T22:49:23Z

force pushed just to fix DCO

Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

…hrono-duration Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

…hrono-duration Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

…hrono-duration Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

wrowe · 2020-08-04T21:37:29Z

@lizan please review, this seems stalled out for your approval, and now for some new clang-tidy chaos on master?

- ambiguous value-based std::chrono::{clock_type}::duration(value) constructors result in stdlib implementation specific default time units which are hard to read and potentially different on different platforms - this change removes any instances of these ambiguous constructions and adds a format check to prevent them; developers should specify an explicit unit of time - we explicitly saw this issue in envoyproxy#11915 where the assumed duration unit was different on Windows, causing test failures Additional Description: Risk Level: Low Testing: Adds format check test and adjust existing unit tests Docs Changes: N/A Release Notes: N/A Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

- ambiguous value-based std::chrono::{clock_type}::duration(value) constructors result in stdlib implementation specific default time units which are hard to read and potentially different on different platforms - this change removes any instances of these ambiguous constructions and adds a format check to prevent them; developers should specify an explicit unit of time - we explicitly saw this issue in envoyproxy#11915 where the assumed duration unit was different on Windows, causing test failures Additional Description: Risk Level: Low Testing: Adds format check test and adjust existing unit tests Docs Changes: N/A Release Notes: N/A Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com> Signed-off-by: chaoqinli <chaoqinli@google.com>

sunjayBhatia mentioned this pull request Jul 22, 2020

router: add config to send date header with requests #11915

Closed

rebello95 reviewed Jul 22, 2020

View reviewed changes

test/extensions/filters/network/kafka/broker/filter_unit_test.cc Outdated Show resolved Hide resolved

tools/code_format/check_format.py Outdated Show resolved Hide resolved

nigriMSFT reviewed Jul 22, 2020

View reviewed changes

tools/code_format/check_format.py Outdated Show resolved Hide resolved

zuercher self-assigned this Jul 22, 2020

sunjayBhatia and others added 2 commits July 22, 2020 17:41

Fix typo and duration in test

bbe9c65

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Convert to milliseconds for consistency

2c7472b

Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

zuercher assigned jmarantz Jul 22, 2020

wrowe and others added 3 commits July 23, 2020 17:33

Further restrict duration ambigutity check

d08f034

- Ensure [Dd]uration is the complete method name - Allow for unambiguous values 0 or 0.0 Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

Merge remote-tracking branch 'origin/master' into fix-ambiguous-std-c…

6fd1b94

…hrono-duration Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Merge remote-tracking branch 'origin/master' into fix-ambiguous-std-c…

18e18b6

…hrono-duration Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

zuercher previously approved these changes Jul 24, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into fix-ambiguous-std-c…

dbdab99

…hrono-duration Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

zuercher assigned lizan and unassigned jmarantz Jul 27, 2020

lizan reviewed Jul 28, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into fix-ambiguous-std-c…

5267929

…hrono-duration Signed-off-by: wrowe@vmware.com

sunjayBhatia added 2 commits July 30, 2020 14:59

Merge remote-tracking branch 'origin/master' into fix-ambiguous-std-c…

bf4174f

…hrono-duration Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia dismissed zuercher’s stale review via bc73785 July 30, 2020 19:06

wrowe mentioned this pull request Jul 30, 2020

Windows: Enable now-passing unit tests #12343

Merged

sunjayBhatia mentioned this pull request Jul 30, 2020

Clean up proliferation of std::chrono duration types #12384

Open

Fix build

12f211a

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

wrowe and others added 6 commits July 31, 2020 09:54

Touch to retest flaky tsan timeout

8be5f83

Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

Merge remote-tracking branch 'origin/master' into fix-ambiguous-std-c…

7d24303

…hrono-duration Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

Change format failure recommendation

4096632

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Tickle CI to get around windows test flake

cf6b0f2

Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

Merge remote-tracking branch 'origin/master' into fix-ambiguous-std-c…

e158b29

…hrono-duration Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

Merge remote-tracking branch 'origin/master' into fix-ambiguous-std-c…

12d9e70

…hrono-duration Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

lizan approved these changes Aug 4, 2020

View reviewed changes

lizan merged commit 0b1cf9d into envoyproxy:master Aug 4, 2020

wrowe deleted the fix-ambiguous-std-chrono-duration branch August 4, 2020 21:52

Conversation

sunjayBhatia commented Jul 22, 2020

Uh oh!

sunjayBhatia commented Jul 22, 2020

Uh oh!

rebello95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zuercher commented Jul 22, 2020

Uh oh!

zuercher commented Jul 22, 2020

Uh oh!

sunjayBhatia commented Jul 24, 2020

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

zuercher commented Jul 27, 2020

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

wrowe commented Jul 28, 2020

Uh oh!

lizan commented Jul 28, 2020

Uh oh!

sunjayBhatia commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lizan commented Jul 30, 2020

Uh oh!

wrowe commented Jul 30, 2020

Uh oh!

sunjayBhatia commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lizan commented Jul 30, 2020

Uh oh!

sunjayBhatia commented Jul 30, 2020

Uh oh!

wrowe commented Aug 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sunjayBhatia commented Jul 29, 2020 •

edited

Loading

sunjayBhatia commented Jul 30, 2020 •

edited

Loading