Restructure windows service template #4

adamralph · 2017-11-13T14:11:29Z

@seanfarmar @bording this is one opinion on how the templates could look. I've only changed the NServiceBusWindowsService template so far.

adamralph · 2017-11-13T14:46:48Z

The build is failing because I haven't updated the approval tests, but I'd like this to be reviewed before proceeding any further.

bording · 2017-11-13T22:25:44Z

src/ParticularTemplates/ParticularTemplates.csproj

@@ -5,6 +5,7 @@
    <Description>dotnet new templates targeting Particular tools and libraries</Description>
    <IncludeBuildOutput>false</IncludeBuildOutput>
    <PackageType>Template</PackageType>
+    <LangVersion>latest</LangVersion>


Why add this? There's no code in this project, though I guess it doesn't hurt to have it. We could add this and TreatWarningsAsErrors to Directory.Build.props instead. We'd have to consider that it would propagate to the template projects by default though.

There is code in this project due to:

<ItemGroup> <None Include="..\Templates\**\*.*" Exclude="**\bin\**\*.*;**\obj\**\*.*" Pack="true" PackagePath="content" /> </ItemGroup>

Indeed, when you open the solution, this is the project that is opened.

I added it because I wanted to use async main etc. but I'll address that consideration in response to your other comment.

There is no compiled code. Note that the include is for the None group.

I missed that. 😄

OK, in that case I'll remove this element, since it's serving no purpose.

bording · 2017-11-13T22:27:09Z

src/Templates/NServiceBusWindowsService/NServiceBusWindowsService.csproj

@@ -4,6 +4,7 @@
    <OutputType>Exe</OutputType>
    <TargetFramework>net462</TargetFramework>
    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
+    <LangVersion>latest</LangVersion>


Should we be setting this in our template project? I'm wondering if we should even be setting TreatWarningsAsErrors here.

As mentioned, I added this because I wanted to use async main.

bording · 2017-11-13T22:27:49Z

src/Templates/NServiceBusWindowsService/Program.cs

+            Console.CancelKeyPress += (sender, e) => { tcs.SetResult(null); };
+
+            await host.Start();
+            await Console.Out.WriteLineAsync("Press Ctrl+C to exit...");


Why Console.Out?

I prefer to be explicit about whether I am writing to stdout or stderr. Also, Console.Out gives us WriteLineAsync(), where as Console only gives us WriteLine(). I guess it doesn't make much difference, but given that we are already in an async context, we may as well use the async method.

However, I don't have a strong preference here. Happy to switch to Console.WriteLine() if that is preferred.

bording · 2017-11-13T23:56:48Z

src/Templates/NServiceBusWindowsService/Program.cs

+
+namespace NServiceBusWindowsService
+{
+    internal static class Program


All of the internal access modifiers seem unnecessary.

I prefer to always be explicit. I don't like reading code and having to mentally map back to the specific defaults for each type of member. E.g. for classes it's internal, for fields and method it's private, etc.

Incidentally, this is also the preference of the dotnet org:

We always specify the visibility, even if it's the default (i.e.
private string _foo not string _foo). Visibility should be the first modifier (i.e.
public abstract not abstract public).

However, if @Particular/host-maintainers prefer not to restate the defaults then I'll remove them.

My vote is to remove them, primarily because none of the default Microsoft templates include them, and I'd like to see our stuff match the default templates as much as possible.

OK, that's a reasonable argument. I'll remove them.

Can we come up with a company wide approach to this? I'm too old to remember settings like this on a repo basis. And no I won't install R# 😃

I'm generally 👎 to company wide standards, but I do think the code should tell us what the standard for a repo is, without imposing a local tooling choice (e.g. R#). It could be done via EditorConfig (now built into VS), Roslyn analyzers, etc.

bording · 2017-11-13T23:57:47Z

src/Templates/NServiceBusWindowsService/Program.cs

+{
+    internal static class Program
+    {
+        public async static Task Main(string[] args)


While using async Main is nice, do we want our templates to have a C# 7.1 dependency?

It's a good point, and I'm glad you brought it up. I committed this with some doubt and I was hoping that someone would call it out. 😄

Perhaps C# 7.1 is too much for now. What would we find acceptable? 7.0? 6.0? Interestingly, while we should ensure that the templates comply with the minimum version we want to support, the content of the langVersion tag in the templates is another matter. E.g. it could still say latest.

Given that we are using SDK-style csproj, the oldest version of VS supported is 2017, and that comes with C# 7.0, so I propose that we change language version to C# 7.0.

bording · 2017-11-14T00:01:48Z

.editorconfig

@@ -0,0 +1,7 @@
+root = true


What do all of these editorconfig values do?

They set the preference to not use this.. It's only a partial representation of the preferences for this repo, but it's a start. The reason I added these specific settings is to override my global machine settings which prefer this. I can remove it from this PR if preferred. In fact, I will, since it's an unrelated change.

adamralph · 2017-11-14T09:31:44Z

@bording I amended the commit to remove the addition of .editorconfig.

adamralph · 2017-11-15T08:06:11Z

I amended the commit to remove the addition of <LangVersion>latest</LangVersion> to ParticularTemplates.csproj.

adamralph · 2017-11-15T09:00:39Z

I pushed a fixup to change the lang version to C# 7 and updated the approved files. This is still a WIP because the ScAdapterService is yet to be changed.

adamralph · 2017-11-15T09:45:51Z

I pushed another fixup to fix line endings 🙄

adamralph · 2017-11-15T09:50:12Z

@seanfarmar and I worked on this this morning and we extracted the common endpoint settings into a file which should ultimately be shared with all endpoints.

bording · 2017-11-15T22:51:27Z

src/Templates/NServiceBusWindowsService/NServiceBusWindowsService.csproj

@@ -4,6 +4,8 @@
    <OutputType>Exe</OutputType>
    <TargetFramework>net462</TargetFramework>
    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
+    <!-- TODO: consider using a later version of C# -->


Hmm, not sure how I feel about this, vs just using the default behavior.

Given that the template is using SDK-style csproj, which requires VS 2017, and C# 7 is lowest possible default in VS 2017, this is redundant, so I'll remove it.

bording · 2017-11-15T22:53:03Z

src/Templates/NServiceBusWindowsService/EndpointConfigurationExtensions.cs

+namespace NServiceBusWindowsService
+{
+    // TODO: move this into a shared project for use in all endpoints
+    internal static class EndpointConfigurationExtensions


I kind of feel like this is going a bit too far with the template.

Agreed, IMO we should remove it

My personal preference is to keep it. It leads the user down the right path for when they add the next project to the solution, and it makes the Host.Start() code read very nicely.

bording · 2017-11-15T22:54:11Z

src/Templates/NServiceBusWindowsService/EndpointConfigurationExtensions.cs

+                // TODO: choose a durable transport for production
+                // https://docs.particular.net/transports/
+                var transportExtensions = endpointConfiguration.UseTransport<LearningTransport>();
+                applyEndpointSpecificSettings?.Invoke(transportExtensions);


Why is this being invoked here specifically? Why not after everything else in here has been called?

Good question, and I have no good answer. I'll move it to the end.

bording · 2017-11-15T22:57:46Z

One other thing to consider, though perhaps in another PR, The ServiceBase class is now available for .NET Core when running on windows: https://www.nuget.org/packages/System.ServiceProcess.ServiceController/4.5.0-preview1-25914-04

So we could extend this template to work with netcoreapp2.0 as well. Maybe it would be better to wait until the package is not prerelease?

andreasohlund · 2017-11-16T09:41:55Z

src/Templates/NServiceBusWindowsService/Host.cs

+
+namespace NServiceBusWindowsService
+{
+    internal class Host


Is there a reason to keep all the redundant internal and private modifiers around?

I prefer to always have them, but @bording makes a good case for removing them in the template, so I'm going to remove them.

andreasohlund · 2017-11-16T09:44:50Z

src/Templates/NServiceBusWindowsService/Host.cs

+
+        private async Task OnCriticalError(ICriticalErrorContext context)
+        {
+            // TODO: decide if stopping the endpoint and exiting the process is the best response to a critical error


Perhaps add a link to https://docs.particular.net/nservicebus/hosting/windows-service?version=core_7#installation-restart-recovery to remind the users that they should setup Service Recovery?

mauroservienti · 2017-11-22T15:49:59Z

src/Templates/NServiceBusWindowsService/EndpointConfigurationExtensions.cs

+namespace NServiceBusWindowsService
+{
+    // TODO: move this into a shared project for use in all endpoints
+    internal static class EndpointConfigurationExtensions


Agreed, IMO we should remove it

mauroservienti · 2017-11-22T15:51:49Z

src/Templates/NServiceBusWindowsService/Host.cs

+        private IEndpointInstance endpoint;
+
+        // TODO: give the endpoint an appropriate name
+        public string EndpointName => "MyNServiceBusWindowsService";


Shouldn't this be a get only property instead of a public field? e.g.:

public string EndpointName{ get; } => "MyNServiceBusWindowsService";

public string Foo => "Bar"; is already a get-only property. That is why there is an expression rather than an assignment. public string Foo { get; } => "Bar"; is invalid syntax. You can have public string Foo { get; } = "Bar"; but that is an initial assignment, rather than an expression body.

yeah, I was writing from the phone and I just copy / pasted adding the {get;}.

public string Foo => "Bar"; is already a get-only property.

Now I have one more reason to dislike expression bodied members 😄

mauroservienti · 2017-11-22T15:53:37Z

src/Templates/NServiceBusWindowsService/Host.cs

+            }
+        }
+
+        private async Task ExitProcess(string message, Exception exception)


I'd prefer to rename this to something more meaningful, as a caller I do not expect ExitProcess to log a fatal and then fail fast.

adamralph

@andreasohlund @bording @mauroservienti I squashed the pre-review commits (the fixups were getting complicated) and I pushed a new fixup which hopefully takes care of the review comments.

Please see my inline comments and advise.

There is also the remaining question of whether we want the separate EndpointConfigurationExtensions class or not. Personally I prefer it, since it guides the user to the pit of success for when the next project is added to the solution, and it also makes for good reading in the Host:

endpointConfiguration.ApplySettingsForAllEndpoints(transport =>
{
    // TODO: apply endpoint-specific transport settings
});

Perhaps we just need to vote on whether to keep it? Currently myself and @seanfarmar are 👍, @mauroservienti is 👎, and @bording sounds undecided but leaning towards 👎 (@bording can you please confirm your opinion?).

adamralph · 2017-11-22T16:18:13Z

src/Templates/NServiceBusWindowsService/Host.cs

+            }
+        }
+
+        async Task LogFatalAndFailFast(string message, Exception exception)


The name is a bit verbose. I'll gladly change it if we can think of something better.

Maybe just FailFast? Does the fact that it also logs need to be in the name?

It was a reaction to @mauroservienti's comment:

I'd prefer to rename this to something more meaningful, as a caller I do not expect ExitProcess to log a fatal and then fail fast.

I made it completely explicit to remove any ambiguity. However, I'm happy with FailFast and in fact I did think about calling it that in my last commit. I'll make that change.

adamralph · 2017-11-22T16:30:11Z

src/Templates/NServiceBusWindowsService/Host.cs

+            await Task.CompletedTask;
+
+            // TODO: https://docs.particular.net/nservicebus/hosting/windows-service#installation-restart-recovery
+            Environment.FailFast(message, exception);


I previously had this in a finally, with the rest of the method in a try, but because this is a FailFast(), it would mask any exceptions thrown in the try, effectively causing them to be swallowed. However, that means that if there is an exception in the log flushing, that error will bubble out of the process instead of the exception that caused the problem in the endpoint, and if we have to choose between the two, we probably want the exception from the endpoint rather than the exception from the log flushing, so perhaps I should re-instate the try...finally?

What exceptions would be swallowed? What else would be in this method that you're concerned about? If the logger throws when we're shutting down anyway, that seems fine to me.

The problem is, that if the logging framework throws then that exception will be thrown out to the runtime as the reason that the process stopped, and the exception that actually caused the endpoint to shutdown in the first place, while still being a first chance exception and therefore perhaps being logged somewhere, is effectively being swallowed. I think it may be better for me to re-instate the try..finally, so that the log fatal/log flushing becomes "best effort" and we ensure that the exception being passed to the method is the one that the runtime sees as causing the process to stop via the fail-fast.

bording · 2017-11-23T01:53:35Z

@adamralph I've taken another look at the EndpointConfigurationExtensions class, and I'm 👎 for keeping it. The fact that the second parameter has the learning transport baked in is worrying, since that's one more place users would have to modify.

I think we'd be better off moving all of that back into the single place, and we can have another comment that says something about considering sharing the configuration if they have more than one endpoint.

seanfarmar

I like this approach, I would merge this and maybe do another round or refactoring later

adamralph · 2017-11-24T17:01:16Z

The fact that the second parameter has the learning transport baked is worrying, since that's one more place users would have to modify.

@bording that's a valid point and, on reflection, I don't like it either. I'm changing my opinion on the EndpointConfigurationExtensions and given that we now have a majority 👎 vote, I'm going to remove it.

adamralph · 2017-11-24T18:01:01Z

I've pushed another fixup. Note that ScAdapterService is still unchanged in this PR so it also needs work to close this out.

andreasohlund · 2017-11-27T09:24:54Z

I've pushed another fixup. Note that ScAdapterService is still unchanged in this PR so it also needs work to close this out.

Given that very few users is expected to use the ScAdapter I'd say we treat this as a separate PR

andreasohlund

I assume we don't use .ConfigureAwait(false) since we know this is a windows service

https://stackoverflow.com/a/23515178/236004

andreasohlund · 2017-11-27T12:19:58Z

@mauroservienti seems like your comments have been addressed?

mauroservienti · 2017-11-27T13:31:00Z

I've pushed another fixup. Note that ScAdapterService is still unchanged in this PR so it also needs work to close this out.

Do we want to address ScAdapterService in this PR? I'm 👎, we should raise another PR.

seems like your comments have been addressed?

yup, approved.

bording · 2017-11-27T23:10:05Z

src/Templates/NServiceBusWindowsService/ProgramService.cs

-                // https://docs.particular.net/nservicebus/serialization/
-                endpointConfiguration.UseSerialization<NewtonsoftSerializer>();
-
-                endpointConfiguration.DefineCriticalErrorAction(OnCriticalError);


This line seems to have disappeared from the new version of template, so OnCriticalError will never be called now.

Good catch @bording !

bording · 2017-11-27T23:12:29Z

src/Tests/TemplateTests.NServiceBusWindowsService.approved.txt

+
+                // TODO: when using an external logging framework it is important to flush any pending entries prior to calling FailFast
+                // https://docs.particular.net/nservicebus/hosting/critical-errors#when-to-override-the-default-critical-error-action
+                await Task.CompletedTask;


Why are we awaiting a Task.CompletedTask here? Why is the method async? Why not just return void here?

Yea lets make this void.

One argument could be to prep for loggers becoming async but aspnet/Logging#444 is a good description on why that's not that likely

bording · 2017-11-27T23:14:55Z

@mauroservienti Since this PR is from @adamralph's fork, If he didn't give us rights to push back to his branch, we might need to open a new PR to deal with additional changes.

Alternatively, we go ahead and merge this as-is, and then fix the remaining items in a new PR.

andreasohlund · 2017-11-28T07:09:22Z

Adam is off this week so lets merge and fix in a separate PR?

bording · 2017-11-28T20:07:36Z

Looks like I was able to push changes to his fork, so I went ahead and fixed up the final items.

bording · 2017-11-28T20:24:05Z

One additional minor tweak: Since the template engine replaces all instances of "NServiceBusWindowsService", the endpoint name also gets changed, so there's no need for the "My" in the name.

adamralph self-assigned this Nov 13, 2017

adamralph requested review from seanfarmar and bording November 13, 2017 14:11

adamralph mentioned this pull request Nov 13, 2017

Report CI status back to pull requests #5

Closed

adamralph changed the title ~~restructure~~ [WIP] restructure Nov 13, 2017

bording reviewed Nov 14, 2017

View reviewed changes

bording reviewed Nov 15, 2017

View reviewed changes

andreasohlund reviewed Nov 16, 2017

View reviewed changes

restructure NServiceBusWindowsService

63f1bb9

mauroservienti requested changes Nov 22, 2017

View reviewed changes

fixup! restructure NServiceBusWindowsService

4151681

adamralph commented Nov 22, 2017

View reviewed changes

seanfarmar previously approved these changes Nov 24, 2017

View reviewed changes

fixup! restructure NServiceBusWindowsService

637edfe

adamralph dismissed seanfarmar’s stale review via 637edfe November 24, 2017 17:59

andreasohlund changed the title ~~[WIP] restructure~~ Restructure windows service template Nov 27, 2017

andreasohlund previously approved these changes Nov 27, 2017

View reviewed changes

mauroservienti previously approved these changes Nov 27, 2017

View reviewed changes

bording requested changes Nov 27, 2017

View reviewed changes

bording added 2 commits November 28, 2017 14:57

Fix line endings

a6fa90e

Add missing DefineCriticalErrorAction call

ad1109e

bording dismissed stale reviews from mauroservienti and andreasohlund via ad1109e November 28, 2017 19:58

bording added 2 commits November 28, 2017 15:02

Make FailFast non-async

4a032c0

Update approval files

e0cc717

bording previously approved these changes Nov 28, 2017

View reviewed changes

No need for "My" in the endpoint name since value is replaced

6bcc8a9

bording dismissed their stale review via 6bcc8a9 November 28, 2017 20:19

bording approved these changes Nov 28, 2017

View reviewed changes

andreasohlund approved these changes Nov 28, 2017

View reviewed changes

andreasohlund merged commit c7347b7 into Particular:develop Nov 28, 2017

bording mentioned this pull request Nov 28, 2017

Restructure ScAdapterService #6

Merged

adamralph deleted the restructure branch December 5, 2017 07:33

Restructure windows service template #4

Restructure windows service template #4

Conversation

adamralph commented Nov 13, 2017

adamralph commented Nov 13, 2017

Choose a reason for hiding this comment

adamralph Nov 14, 2017 • edited

Choose a reason for hiding this comment

bording Nov 14, 2017 • edited

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

adamralph Nov 14, 2017 • edited

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

adamralph commented Nov 14, 2017

adamralph commented Nov 15, 2017

adamralph commented Nov 15, 2017

adamralph commented Nov 15, 2017

adamralph commented Nov 15, 2017 • edited

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

bording commented Nov 15, 2017

Choose a reason for hiding this comment

adamralph Nov 22, 2017 • edited

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

adamralph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamralph Nov 24, 2017 • edited

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

bording commented Nov 23, 2017 • edited

seanfarmar left a comment

Choose a reason for hiding this comment

adamralph commented Nov 24, 2017

adamralph commented Nov 24, 2017

andreasohlund commented Nov 27, 2017

andreasohlund left a comment

Choose a reason for hiding this comment

andreasohlund commented Nov 27, 2017

mauroservienti commented Nov 27, 2017

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

bording commented Nov 27, 2017

andreasohlund commented Nov 28, 2017

bording commented Nov 28, 2017

bording commented Nov 28, 2017

adamralph Nov 14, 2017 •

edited

bording Nov 14, 2017 •

edited

adamralph Nov 14, 2017 •

edited

adamralph commented Nov 15, 2017 •

edited

adamralph Nov 22, 2017 •

edited

adamralph Nov 24, 2017 •

edited

bording commented Nov 23, 2017 •

edited