Refactor http to reduce static singleton dependencies #1310

MarkZuber · 2018-10-09T01:29:29Z

No description provided.

henrik-me · 2018-10-10T03:16:29Z

core/src/Platforms/iOS/iOSPlatformProxy.cs

+        public ICryptographyManager CreateCryptographyManager()
+        {
+            return new iOSCryptographyManager();
+        }


thinking... should this be cached? We considering the same for some of the other objects. however perhaps this doesn't pay off as those are relatively cheap objects to create. #Closed

yes, I've changed these to be class initialized properties so they're only constructed once. #Closed

henrik-me · 2018-10-10T03:17:17Z

Looks great! Added a few points... most of them probably just for a quick chat. #Closed

henrik-me · 2018-10-10T03:18:33Z

.../src/Microsoft.IdentityModel.Clients.ActiveDirectory/Internal/Platform/IHttpClientFactory.cs

-    }
+    //interface IHttpClientFactory
+    //{
+    //    IHttpClient Create(string uri, RequestContext requestContext);


remove completely? #Closed

fixed #Closed

henrik-me · 2018-10-10T03:22:24Z

core/src/PlatformProxyFactory.cs

+#elif IS_MSAL
+            return true;
+#else
+            return true;  // TODO: should we default to MSAL or should we default to throw exception?


throw exception? [](start = 85, length = 16)

IMO: Throw. should not ever happen #Closed

fixed #Closed

henrik-me · 2018-10-10T03:24:52Z

core/src/IPlatformProxy.cs

+
+        ILegacyCachePersistance CreateLegacyCachePersistence();
+        ITokenCacheAccessor CreateTokenCacheAccessor();
+        ICryptographyManager CreateCryptographyManager();


I made most of my comments on the "Create" pattern in the implementation class. Adding here that perhaps we should follow the lazy pattern used in the platform proxy class to create the platform information objects instead of newing "helper"objects every time? Perhaps the gain is neglectable.
My thinking is not for the client side scenarios but rather server side scenarios. #Closed

I've fixed this. please check updated code once I push. #Closed

henrik-me · 2018-10-10T03:27:41Z

            return version[1];

Outside the scope of the refactoring,,, but considering introducing caching here... using well, a static given the class is static.
perhaps it would be worth caching this value? no need for every telemetry entry to re-calculate this value.
thoughts?

Aside: perhaps this should really be moved to the AssemblyUtils class?

[mzuber] -- I'm going to make a note in my refactoring onenote to look at cleaning up this class since I agree it's weird for us to recalculate this stuff constantly. but i'm going to push it out of this PR and do it later. #Pending

Refers to: core/src/MsalIdHelper.cs:111 in e980b31. [](commit_id = e980b31, deletion_comment = False)

henrik-me · 2018-10-10T03:59:20Z

msal/src/Microsoft.Identity.Client/Features/ConfidentialClient/ConfidentialClientApplication.cs

-
+using System.Threading;
+using Microsoft.Identity.Core.Http;
+
 namespace Microsoft.Identity.Client
 {
    /// <summary>


///
[](start = 4, length = 13)

was thinking if we can do inherit docs for both PBA and CCA? #Pending

Yes, I want to make another documentation cleanup pass. Added notes to my backlog. #Pending

henrik-me · 2018-10-10T04:06:29Z

msal/src/Microsoft.Identity.Client/Internal/ModuleInitializer.cs

    /// use this it would require IL weaving, which does not seem to work on all target frameworks.
    /// Instead, call <see cref="EnsureModuleInitialized"/> from static ctors of public entry points.
    /// </remarks>
    internal class ModuleInitializer
    {
-        private static bool isInitialized = false;
-        private static readonly object lockObj = new object();
+        private static bool _isInitialized = false;


_isInitialized [](start = 28, length = 14)

should this be volatile ? #Closed

yes. fixed. #Closed

henrik-me · 2018-10-10T04:07:11Z

msal/src/Microsoft.Identity.Client/Internal/MsalLogger.cs

@@ -137,7 +137,7 @@ private void Log(LogLevel msalLogLevel, string messageWithPii, string messageScr
                ? string.Empty
                : " - " + CorrelationId;

-            var msalIdParameters = MsalIdHelper.GetMsalIdParameters(new PlatformInformation());
+            var msalIdParameters = MsalIdHelper.GetMsalIdParameters();


msalIdParameters [](start = 16, length = 16)

can this be "cached" for the lifetime of logger? perhaps even injected at creation time (or static ctr) #Closed

yes, it can be. backlog of notes for future refactor. #Pending

henrik-me · 2018-10-10T04:08:03Z

msal/src/Microsoft.Identity.Client/Internal/PlatformInformationBase.cs

-    }
+    //internal abstract class PlatformInformationBase : CorePlatformInformationBase
+    //{
+    //}


remove completely ? #Closed

fixed #Closed

henrik-me · 2018-10-10T04:10:02Z

some of these change reminds me .... I feel we are missing micro/macro performance tests which can quickly help assess whether we will improve especially service performance... which is overall my biggest concern for the library when it comes to performance and memory usage #Pending

henrik-me · 2018-10-10T04:14:58Z

msal/tests/Test.MSAL.NET.Unit/ConfidentialClientApplicationTests.cs

+                Assert.AreEqual("MSAL.Desktop", qp["x-client-sku"]);
+                Assert.IsFalse(string.IsNullOrEmpty(qp["x-client-ver"]));
+                Assert.IsFalse(string.IsNullOrEmpty(qp["x-client-cpu"]));
+                Assert.IsFalse(string.IsNullOrEmpty(qp["x-client-os"]));


seems like most of the validation stuff is the same in more places. ... seems duplicate and makes me think that some refactorings are needed and perhaps the tests are not testing different things... #Closed

yes, i had similar feelings on copy/paste and duplication across many of the tests, this being a prime example. test cleanup and organization is also in my backlog. #Pending

henrik-me · 2018-10-10T04:19:59Z

core/tests/Test.Microsoft.Identity.Core.Unit/Mocks/MockHttpManager.cs

+            // even though it's not the root cause.
+            if (Marshal.GetExceptionCode() == 0)
+            {
+                Assert.AreEqual(0, _httpMessageHandlerQueue.Count, "All mocks should have been consumed");


Assert.AreEqual(0, _httpMessageHandlerQueue.Count, "All mocks should have been consumed"); [](start = 15, length = 91)

interesting approach,

nit as it is easy to attach a debugger...: thought perhaps list the handlers which are still in the queue... to make debugging easier? #Closed

Given that the mocks are added in order in the tests (so it's easy to find the last one or two if they're missed), and the handlers don't "tostring" very cleanly i don't think this is worth the effort here. but let's see as we develop and if we can make improvements here we certainly will. #Closed

henrik-me · 2018-10-10T04:23:17Z

<wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation">

Is this the netcore ruleset?
Should we have the same for the one with samples? .... would have liked the same for vs settings... as I do not currently use resharper.

[mzuber] -- this is very close to the netcore ruleset but i didn't copy it from there (we definitely can do that instead). VS doesn't do quite enough yet to get code formatting consistently so I'm doing this to get some of the major cleanup points done automatically. and we can tweak the ruleset and see what we can do in plain vanilla VS so we can get more consistent here. #Pending

Refers to: LibsNoSamples.sln.DotSettings:1 in e980b31. [](commit_id = e980b31, deletion_comment = False)

henrik-me

🕐

bgavrilMS · 2018-10-10T14:07:09Z

core/src/PlatformProxyFactory.cs

+        {
+        }
+
+        private static bool IsMsal()


I think PlatformProxy does too much now. My initial refactoring idea was to break up the old "PlatformInformation" into a PlatformProxy that focuses only on OS and .net flavour (read the upn from the OS etc.) and a LibraryProxy that would handle things like "get version". #Pending

I'd like to proceed with the current refactor since all of the code being moved in here is paltform specific. Personally, I like the idea of having some root object (like platform proxy) that gives is the central core for how we do platform disambiguation. But if we want to break this down into smaller parts hanging off platformproxy or something else, let's look into that. #Pending

… since we can't automate the browser

jennyf19 · 2018-10-11T05:33:34Z

...dentityModel.Clients.ActiveDirectory/Internal/Flows/NonInteractive/AcquireTokenIWAHandler.cs

@@ -59,24 +60,24 @@ public AcquireTokenIWAHandler(RequestData requestData, IntegratedWindowsAuthInpu
                SupportADFS = true;
            }

-            this.iwaInput = iwaInput;
+            this._iwaInput = iwaInput;


this [](start = 12, length = 4)

why do you still need this. here?

henrik-me

MarkZuber added 8 commits October 5, 2018 14:25

remove httprequest, restructure all calls to ihttpmanager

789f493

major http refactoring

8baddd2

Merge branch 'dev' into markzuber/http

e874915

more http and unit test cleanup

245bead

fix

5377116

add missing options for platforms

ce97338

Merge branch 'dev' into markzuber/http

a9f7cf0

fix

560a397

MarkZuber requested review from trwalke, jennyf19, henrik-me and bgavrilMS October 9, 2018 18:40

also restructure tokencacheaccessor, legacycachepersistance, and crypto

e980b31

henrik-me reviewed Oct 10, 2018

View reviewed changes

henrik-me suggested changes Oct 10, 2018

View reviewed changes

bgavrilMS reviewed Oct 10, 2018

View reviewed changes

pr feedback

8ffd2de

MarkZuber added 2 commits October 10, 2018 15:04

remove device code integration test since it's marked [Ignore] anyway…

72a11c8

… since we can't automate the browser

dead code

7516e97

jennyf19 reviewed Oct 11, 2018

View reviewed changes

henrik-me approved these changes Oct 11, 2018

View reviewed changes

MarkZuber added 8 commits October 15, 2018 17:08

fix headers

772558d

Merge branch 'dev' into markzuber/http

d9778cf

Merge branch 'dev' into markzuber/http

af91edf

Merge branch 'dev' into markzuber/http

0c99eb6

conf await

0b951f7

clean

eedfed8

miss

3f93a0f

fix

06a1b24

MarkZuber merged commit 93d88f3 into dev Oct 17, 2018

MarkZuber deleted the markzuber/http branch October 17, 2018 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor http to reduce static singleton dependencies #1310

Refactor http to reduce static singleton dependencies #1310

MarkZuber commented Oct 9, 2018 •

edited

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me commented Oct 10, 2018 •

edited

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me commented Oct 10, 2018 •

edited

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me commented Oct 10, 2018 •

edited

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me commented Oct 10, 2018 •

edited

Loading

henrik-me left a comment

bgavrilMS Oct 10, 2018 •

edited by henrik-me

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

jennyf19 Oct 11, 2018

henrik-me left a comment

Refactor http to reduce static singleton dependencies #1310

Refactor http to reduce static singleton dependencies #1310

Conversation

MarkZuber commented Oct 9, 2018 • edited Loading

henrik-me Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

MarkZuber Oct 10, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

henrik-me commented Oct 10, 2018 • edited Loading

henrik-me Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

MarkZuber Oct 10, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

henrik-me Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

MarkZuber Oct 10, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

henrik-me Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

MarkZuber Oct 10, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

henrik-me commented Oct 10, 2018 • edited Loading

henrik-me Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

MarkZuber Oct 10, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

henrik-me Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

MarkZuber Oct 10, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

henrik-me Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

MarkZuber Oct 10, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

henrik-me Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

MarkZuber Oct 10, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

henrik-me commented Oct 10, 2018 • edited Loading

henrik-me Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

MarkZuber Oct 10, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

henrik-me Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

MarkZuber Oct 10, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

henrik-me commented Oct 10, 2018 • edited Loading

henrik-me left a comment

Choose a reason for hiding this comment

bgavrilMS Oct 10, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

MarkZuber Oct 10, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

jennyf19 Oct 11, 2018

Choose a reason for hiding this comment

henrik-me left a comment

Choose a reason for hiding this comment

MarkZuber commented Oct 9, 2018 •

edited

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me commented Oct 10, 2018 •

edited

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me commented Oct 10, 2018 •

edited

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me commented Oct 10, 2018 •

edited

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me commented Oct 10, 2018 •

edited

Loading

bgavrilMS Oct 10, 2018 •

edited by henrik-me

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading