Plat stop breakpoints causing timeouts #41

lewislabs · 2015-07-09T14:09:13Z

Commands now have the facility to ignore the timeouts. This should make it easier to debug Command decorated methods.

Ignoring timeouts can either be configured using the CommandAttribute constructor (in Hudl.Mjonir.Attributes), or using a ConfigurableValue (for system wide ignoring of timeouts).

…its debugging.

…c out of the InvocationCommand and into the base class

…mand

…Attribute

…Value causes an overflow. - Added an extra ctor to the InvocationCommand

…d timeouts from the CommandAttribute - Added a new set of tests for the non-obsolete CommandAttribute. Put in some tests for ignoredTimeouts

lewislabs · 2015-07-09T14:12:00Z

Hudl.Mjolnir.Attributes/CommandAttribute.cs


        // See Mjolnir's Command constructors.
        public CommandAttribute(string group, string isolationKey, int timeout = DefaultTimeout) : this(group, isolationKey, isolationKey, timeout) { }

+        public CommandAttribute(string group, string isolationKey, bool ignoreTimeout)


I've added this extra constructor to allow for setting specific Command calls to ignore timeouts. Is this wise? Do we want to expose this feature?

I don't think we do - ignoring timeouts seems like a conditional thing, so to have it baked into the attribute permanently feels odd.

I'll be honest, part of the reason for me doing this was to make testing a little bit easier. But I can take it out and just use an injected ConfigurationProvider, as is done in the current set of tests.

Yeah, I think that's the way to go - it'll more align the tests with how the library is supposed to behave (i.e. driven by the config value). If we have to go this route, I think we'd want this constructor to be internal so that it's not part of the public API.

…orking some of the code to adjust for that. Tests are broken at this time and need some rework.

… order of test matters to their success

robhruska · 2015-07-10T13:37:32Z

Hudl.Mjolnir.Tests/Command/Attribute/NewCommandAttributeAndProxyTestsIgnoringTimeouts.cs

+
+namespace Hudl.Mjolnir.Tests.Command.Attribute
+{
+    public sealed class NewCommandAttributeAndProxyTestsIgnoringTimeouts : TestFixtureIgnoreTimeouts


The TestFixtureIgnoreTimeouts caught me here - I was reading through the tests and said to myself, "it looks like these aren't actually using the ignoreTimeouts property", and I stumbled upon how it was used later. What are your thoughts on bringing the config value closer to the tests here? It feels too disconnected to me.

We can do that, just means running the TestFixtureIgnoreTimeouts and TestFixture constructor logic at the start of each test.
A lot of changes but In the spirit of the xunit philosophy maybe we should do that.

@robhruska Having started to move this logic to the start of each of the ~160 tests, I'm starting to think it's better where it is in the constructor. Here's my reason;

In each test, since we don't know what other tests may have done to the ConfigProvider, we have to initialise all of the possible keys. e.g. in a test where we're not looking to test anything related to the ignoreTimeouts functionality we still need to start the test off with a

ConfigProviderContext.Instance.SetConfigValue(ConfigProviderContext.IgnoreTimeoutsKey, false);

This seems like code bloat, and is perhaps more confusing than having it all in one place?
It also makes it tricky if we end up adding another config value to the tests!

In each test, since we don't know what other tests may have done to the ConfigProvider

If we assume that other tests appropriately tore themselves down, is that enough?

we still need to start the test off with a ...

If we can assume a sane config state, and the config value defaults to false, we should only have to add it to cases where we want to set it with a true value.

Thoughts there?

I'm thinking that relying on assumptions in a unit test for it to pass, is a little bit flawed. Also if one test forgets to tear down properly then finding where that is could be more tricky.

But on the other hand if we actually changed the default values for config inside of mjolnir in the future, then I would hope a bunch of failing tests would be enough for someone to reconsider what they're doing. This is probably the biggest benefit so I'll go with the approach of just setting values where they are needed and assuming that the world is otherwise configured using defaults.

robhruska · 2015-07-10T13:47:57Z

Hudl.Mjolnir/Command/Command.cs

+                _log.DebugFormat("Timeout configuration override for this command of {0}",timeout);
+                //I think it is sensible to apply this timeout to the command if it has been explicitly set in configuration. 
+                //Therefore setting TimeoutsFullyDisabled to false regardless
+                TimeoutsIgnored = false;


This feels a little hidden to me. As a user of the library, if I set mjolnir.ignoreTimeouts=True, that feels like a pretty assertive thing (like, I expect it to ignore all timeouts). If I were to set the config to True and still encounter a timeout, I'd be confused. Additionally, in local development, I'm almost positive that we have 0 people with per-command timeouts configured, so I'm not sure there's much of a problem to address by doing this.

I guess ignoreTimeouts should be fully self explanatory. I was in two minds as to what I was going to do with these per command timeouts. My thought was that it may give users the ability to apply the blanket ignore first, then if they were interested in tweaking the timeout for a particular command they could just work with the per-command config.
I guess since the per-command config overrides the default anyway you can still get the same use case.

I'll change it.

…at the default ConfigurationValues apply.

robhruska · 2015-07-13T15:13:16Z

Hudl.Mjolnir/Command/Command.cs

@@ -169,7 +177,8 @@ internal Command(string group, string name, string breakerKey, string poolKey, T
                throw new ArgumentNullException("poolKey");
            }

-            if (defaultTimeout.TotalMilliseconds <= 0)
+            TimeoutsIgnored = IgnoreCommandTimeouts.Value;
+            if (!TimeoutsIgnored && defaultTimeout.TotalMilliseconds <= 0)


Technically I don't think we need to check the ignored flag here - the default timeout's often going to get passed from a constant or hard-coded value, so it should be valid.

In fact, thinking about it more, I'd lobby to always validate the default timeout here, because otherwise, if timeouts are ignored, invalid default timeout values might go unnoticed in development.

That's true! I think there's actually a test which looks at this. It will give a false positive in the case where ignoredTimeouts is true and the timeout is <=0

- cleaning up some overly verbose code

ramziyassine · 2015-07-16T15:14:55Z

Hudl.Mjolnir/Command/Command.cs

+        /// This is likely to be useful when debugging Command decorated methods, however it is not advisable to use in a production environment since it disables 
+        /// some of Mjolnir's key features. 
+        /// </summary>
+        protected static readonly ConfigurableValue<bool> IgnoreCommandTimeouts = new ConfigurableValue<bool>("mjolnir.ignoreTimeouts", false);


with Mjolnir being open source are we ok referencing ConfigurableValue

Yeah, we've got a version of Hudl.Config that's published to nuget.org

I haven't made any changes to mjolnir's references

Plat stop breakpoints causing timeouts.

lewislabs added 8 commits July 7, 2015 11:40

Adding the ability to override the use of timeouts in commands. Benef…

0af3535

…its debugging.

Making this slightly easier to test with ctor param

2dc2065

Need to fix this after a rename

2523eba

Added a test for the ignored timeouts and refactored some of the logi…

bab293d

…c out of the InvocationCommand and into the base class

spelling mistake in test name and misplaced cancellation token in com…

7d732e0

…mand

Providing the ability to ignore timeouts directly through the Command…

8eaeff4

…Attribute

-Realised through testing of the CommandInterceptor that TimeSpan.Max…

9b3110b

…Value causes an overflow. - Added an extra ctor to the InvocationCommand

-modified the CommandInterceptor to look at the possibility of ignore…

2ddec34

…d timeouts from the CommandAttribute - Added a new set of tests for the non-obsolete CommandAttribute. Put in some tests for ignoredTimeouts

lewislabs reviewed Jul 9, 2015
View reviewed changes

lewislabs added 3 commits July 9, 2015 19:45

- Removing the ignoreTimeout param from the command attribute and rew…

23c4ac5

…orking some of the code to adjust for that. Tests are broken at this time and need some rework.

We no longer need a new version of Mjonir.Attributes

c993ca6

Modifying tests to use a global ConfigProvider, otherwise the running…

7f57450

… order of test matters to their success

robhruska reviewed Jul 10, 2015
View reviewed changes

Making the use of mjolnir.ignoreTimeouts a little bit clearer

8b4c252

robhruska reviewed Jul 10, 2015
View reviewed changes

lewislabs added 2 commits July 10, 2015 15:58

If timeouts are ignored then this should be a blanket application.

97a8646

All tests should set up config for themselves and otherwise assume th…

c8f2c05

…at the default ConfigurationValues apply.

robhruska reviewed Jul 13, 2015
View reviewed changes

- Still want to check timeout is positive even if timeouts are ignored.

15b2462

- cleaning up some overly verbose code

ramziyassine reviewed Jul 16, 2015
View reviewed changes

lewislabs added a commit that referenced this pull request Jul 20, 2015

Merge pull request #41 from hudl/Plat-StopBreakpointsCausingTimeouts

c93ee9e

Plat stop breakpoints causing timeouts.

lewislabs merged commit c93ee9e into master Jul 20, 2015

robhruska deleted the Plat-StopBreakpointsCausingTimeouts branch July 20, 2015 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plat stop breakpoints causing timeouts #41

Plat stop breakpoints causing timeouts #41

lewislabs commented Jul 9, 2015

lewislabs Jul 9, 2015

robhruska Jul 9, 2015

lewislabs Jul 9, 2015

robhruska Jul 9, 2015

robhruska Jul 10, 2015

lewislabs Jul 10, 2015

lewislabs Jul 10, 2015

robhruska Jul 10, 2015

lewislabs Jul 13, 2015

robhruska Jul 10, 2015

lewislabs Jul 10, 2015

robhruska Jul 13, 2015

lewislabs Jul 13, 2015

ramziyassine Jul 16, 2015

robhruska Jul 16, 2015

lewislabs Jul 16, 2015

Plat stop breakpoints causing timeouts #41

Plat stop breakpoints causing timeouts #41

Conversation

lewislabs commented Jul 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment