Skip to content
This repository was archived by the owner on Jun 20, 2019. It is now read-only.

Adds ANCM in-process flag. #144

Merged
merged 4 commits into from
Sep 11, 2017
Merged

Adds ANCM in-process flag. #144

merged 4 commits into from
Sep 11, 2017

Conversation

jkotalik
Copy link
Contributor

@jkotalik jkotalik commented Sep 1, 2017

Name is up for discussion. @shirhatti I decided a bool is better for now.

@dnfclas
Copy link

dnfclas commented Sep 1, 2017

@jkotalik,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@Tratcher
Copy link
Member

Tratcher commented Sep 1, 2017

Not a fan, it's not extensible. How about hostingMode="inprocess" or "outofprocess"?

@muratg
Copy link
Contributor

muratg commented Sep 1, 2017

@Tratcher What kind of extensibility do you envision? Do we expect to have other kinds of hosting modes in the future?

We had a talk with @shirhatti about ideally making this the last web.config schema change for ANCM, and possibly introduce other ways to configure it (e.g. ANCM.json). We'd like to get to a point that all ANCM updates could be done via a siteextension.

@Tratcher
Copy link
Member

Tratcher commented Sep 1, 2017

Extensibilty such as hostingMode="thenextgreatthing", which would not require a schema change?

@shirhatti
Copy link
Contributor

cc @DamianEdwards

@pan-wang
Copy link
Contributor

pan-wang commented Sep 5, 2017

with in-proc, do we still plan to support other configuration elements, such as environment variables?

@Tratcher
Copy link
Member

Tratcher commented Sep 6, 2017

We should attempt to support as many as possible. The transition between the two modes should be as seamless as possible.

@muratg
Copy link
Contributor

muratg commented Sep 6, 2017

Agreed with @Tratcher. Are there any we can't support?

@davidfowl
Copy link
Member

Yes, we should try to support everything.

@davidfowl
Copy link
Member

I was thinking about this a bit and I like the string thing. You can imagine the string representing the transport type:

  • "http"
  • "inprocess"
  • "namedpipes"
  • "fastcgi"

@Tratcher
Copy link
Member

Tratcher commented Sep 8, 2017

Also, "https", or "http://ip:port" for hard coding the forward ip and port? Some people really don't like our random port mechanics.

@Tratcher
Copy link
Member

Tratcher commented Sep 8, 2017

Maybe it should be named transport instead.

@Tratcher
Copy link
Member

Tratcher commented Sep 8, 2017

hmm, no, transport makes more sense as a separate thing from process model.

@davidfowl
Copy link
Member

@Tratcher I don't care if we call it transport or not I think the same knob should be used if we choose to change the communication mechanism between ANCM and the managed process.

@Tratcher
Copy link
Member

Tratcher commented Sep 8, 2017

Disagree. How you launch the process is rather independent of how you connect to it. The inproc case is the exception. E.g. Ways to launch the process: inproc, outofproc, none (managed externally), etc.

@pan-wang
Copy link
Contributor

pan-wang commented Sep 8, 2017

I prefer hostingModel or processModel. Transport is so misleading for in-proc or out-proc.

@davidfowl
Copy link
Member

davidfowl commented Sep 8, 2017

So we'll do "inprocess" "http" that doesn't make any sense. What's the scenario where having separate options makes sense? Give me something real...

Once again call it mode, I just don't think there's just inproc and outproc. It's about how the module communicates with dotnet. That's what this mode represents

@Tratcher
Copy link
Member

Tratcher commented Sep 8, 2017

Not every combo will make sense, but consider the following pairs:
outofproc + http
outofproc + https
outofproc + http2
outofproc + namedpipe
cluster + http
cluster + https
cluster + http2
cluster + namedpipe
selfstarted + http
selfstarted + https
selfstarted + http2
selfstarted + namedpipe
inproc + direct
inproc + namedpipe

@davidfowl
Copy link
Member

@Tratcher you're making things up now. What is inproc named pipe and inproc direct?

@davidfowl
Copy link
Member

To summarize my position, this new communication "mode" is how ANCM talks to dotnet. Reasons:

  1. It’s simpler
  2. Nothing is broken
  3. No need to invalidate bad combinations (like inproc + named pipes)

The purity argument that they are different things requires that we explode or ignore when you have an invalid combination (like in process + http).

@jkotalik
Copy link
Contributor Author

Based on the feedback, I'm going to change the name to hostingModel and the type to a string. @Tratcher we may want to expose transport eventually, but I think that could be added to some eventual ANCM.json file.

@@ -25,6 +25,7 @@
<attribute name="processesPerApplication" type="uint" defaultValue="1" validationType="integerRange" validationParameter="1,100"/>
<attribute name="forwardWindowsAuthToken" type="bool" defaultValue="true" />
<attribute name="disableStartUpErrorPage" type="bool" defaultValue="false" />
<attribute name="hostingModel" type="string" defaultValue="outofprocess" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you not have a default? e.g. default to empty? Let's handle the default in code.

@pan-wang
Copy link
Contributor

I understand the motivation that you want to make the new string attribute as a generic solution and thus no default value given, though I don't like it as it looks we don't know what possible values could be.

@pan-wang
Copy link
Contributor

@jkotalik you can merge this schema change now now

@jkotalik jkotalik merged commit 509ddc6 into dev Sep 11, 2017
@davidfowl
Copy link
Member

default should be http

@jkotalik jkotalik deleted the jkotalik/Schema branch September 29, 2017 20:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants