[Do Not Merge] Refactor RuntimeFramework for thread-safety #229
Conversation
| /// <param name="version">The version of the framework</param> | ||
| public RuntimeFramework(RuntimeType runtime, Version version) | ||
| : this(runtime, version, null) | ||
| { |
There was a problem hiding this comment.
Used an optional parameter instead of additional constructor
| @@ -108,6 +98,9 @@ public RuntimeFramework(RuntimeType runtime, Version version, string profile) | |||
| Profile = profile; | |||
|
|
|||
| DisplayName = GetDefaultDisplayName(runtime, FrameworkVersion, profile); | |||
|
|
|||
| if (clrVersion != null) | |||
| ClrVersion = clrVersion; | |||
There was a problem hiding this comment.
This is messy, and solely here to retain previous functionality. ClrVersion is set differently depending on if the framework is found as a "CurrentFramework" or an "AvailableFramework". I'd like to standardise that, but it would be a change to the tests, so I can do that separately later.
| } | ||
| else | ||
| displayName = "Mono " + displayName; | ||
| } |
There was a problem hiding this comment.
Mono display names were previously set differently depending if it was a CurrentFramework or AvailableFramework. I moved this functionality here mainly for accessibility reasons, although it would be good to also resolve this inconsitency.
| // TODO: This was left where it always has been - probably wants to be moved. | ||
| if (RuntimeFramework.MonoPrefix == null) | ||
| RuntimeFramework.MonoPrefix = GetMonoPrefixFromAssembly(monoRuntimeType.Assembly); | ||
|
|
There was a problem hiding this comment.
This is where the mono display name creation functionality from above previously was, which is why MonoPrefix is randomly set all on it's own, here. The Mono* properties on RuntimeFramework are currently set all over - this all will want refactoring as a later part of this PR.
| @@ -62,30 +81,6 @@ public bool IsAvailable(string name) | |||
| return false; | |||
| } | |||
|
|
|||
| private static readonly Version AnyVersion = new Version(0, 0); | |||
There was a problem hiding this comment.
This field/method have just been moved within the class, to tidy up a little.
|
|
||
| namespace NUnit.Engine.Services.FrameworkUtilities | ||
| { | ||
| internal static class DotNetFrameworkLocator |
There was a problem hiding this comment.
This is unchanged content wise - just minor changes to method returns to allow this class to be solely static methods.
|
Currently, in the gui we only use |
|
Great - thanks @mikkelbu! In that case, this would be a breaking change for the GUI - however, it is already noted as a TODO for the GUI, that this is the wrong way to integrate. RuntimeFrameworkService already has I've broke the mono tests, but ran out of time for today - I'll get back to it. |
|
I like the overall direction. AFAIC it's not an "official" Breaking Change if we break folks that are using the engine directly. However we do want to be able to test these classes and AFAIK we are not using InternalsVisibleTo. As an old-timer, I'm comfortable with making things public for test purposes. After all, some of the original OO languages didn't even have the notion of visibility and some of the current interpreted languages don't either. However, I'm OK with internal so long as we first release with the functionality needed in the interface, so runners can make the change. |
|
Code looks good but I'm not comfortable with having public static methods in a service. A service is supposed to be accessed through it's interface. Utility methods are a better place for statics like CurrentFramework. In the case of AvailableFrameworks, I would add it to the interface and make it available to runners. |
|
Thanks @CharliePoole - that's really helpful.
We already have InternalsVisibleTo set up. I prefer to use it when available - although I agree it's less important for the engine, where it shouldn't be being referenced directly anyway.
Would you prefer me to make the interface changes in a separate PR first? Happy to do that.
I'm glad you picked up on this! I kept the current methods as statics as all the services currently are independent, and I didn't want to start passing services into each other. e.g. TestAgency uses CurrentFramework, and it didn't seem right to pass IRuntimeFrameworkService into TestAgency. I'm guessing TestAgency should instead take a IServiceLocator, so it can find the IRuntimeFrameworkService? |
|
Actually, there are already a few inter-service dependencies, some hard, some soft. It affects order of initialization, which we handle by manually controlling the order of adding the services. I've thought of making the dependencies explicit and the initialization smarter but so far it hasn't seemed worth it. |
|
This is still a WIP, as its much bigger than I expected. I'll be breaking it up into multiple PRs, the first of which is #249 I hope to get these PRs done between 3.7 and 3.8, as I'm a little concerned that the 'intermediate state' may produce subtle parallilisation issues, but hopefully the end result will be more stable. |
|
Multiple PRs sounds good. Of the items listed, I think CurrentFramework and AvailableFrameworks should be in the API. The GUI would be helped by this - it is currently forced to use RuntimeFramework directly. The MonoXxxxxx properties seem internal only to me, along with GetBestAvailableFramework(). |
|
#249 exposes AvailableFrameworks, and this PR will expose CurrentFramework once other things are merged - as this one will contain the content to move CurrentFramework to the RuntimeFrameworkService. I don't believe the GUI currently uses CurrentFramework, although I agree about exposing it - especially as other services can then locate an IRuntimeFrameworkService. |
|
I'm closing this one. I've left the code to rot too long, and it's a much bigger job than I first envisaged. While it's not causing any current problems, I can't commit the time to it. |
This is a work-in-progress that starts on #173. It would be good to get feedback on the current direction.
RuntimeFramework was originally written for a single-threaded environment, and wants refactoring for use in a multi-threaded world. There's a number of hacks in place to support it's current use, and it's content doesn't particularly fit with the current architecture. This is blocking #140 and #228.
My intention is to reduce RuntimeFramework to solely hold information about a single framework. I want to move most of the other functionality to RuntimeFrameworkService, which will also help with thread-safety, and ensure properties are just initialised the once. I also want to add some utility classes for some of the framework detection/location - just as the current classes are pretty big, and it would be nice to break them up.
I'm looking to move out:
All of this is currently
public- my thinking is that most should be internal, except CurrentFramework and AvailableFrameworks, which should expose IRuntimeFramework objects via IRuntimeFrameworkService.I don't currently believe this will cause any breaking changes to runners using nunit.engine.api - however, any runners which integrate directly with the engine will see breaking changes. I don't know if the GUI currently uses these classes? And more generally - are we happy with 'breaking changes' here? My personal thoughts are that getting the architecture right is worthwhile here.
I'd suggest reviewing commit-by-commit. This should almost entirely be just moving code around - I'll comment any intended changes. I'll stop here until there's some feedback on the direction, as moving the mono content was much more messy!