Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Port ANCM current directory changes to 2.2 #6150

Merged
merged 3 commits into from
Jan 15, 2019
Merged

Port ANCM current directory changes to 2.2 #6150

merged 3 commits into from
Jan 15, 2019

Conversation

pakrym
Copy link
Contributor

@pakrym pakrym commented Dec 27, 2018 •
edited

Ports #6066 #4798 #4369

Description:

In InProcess hosting mode process current directory is not set to application physical path location like it was in OutOfProcess causing hard to diagnose exception in Program.Main for a lot of customers. (#6118 #6117 #4206 #5961 huorswords/Microsoft.Extensions.Logging.Log4Net.AspNetCore#55 and more)

This change makes process current directory to be application physical path by default.

Regression?

Regression from OutOfProcess hosting mode.

Risk:

Medium-Low. It's hard to predict all the effects changing current directory might have on IIS worker process but testing didn't show any negative ones.
Some things to consider:

  1. IIS Express and Hostable Web Core that share most of the code with w3wp support working from any current directory
  2. We reset DllDirectory to point to the original CurrentDirectory to restore dll lookup order.
  3. There is a handlerSetting option to opt-out of new behaviour.
  4. This change was in master for some time now and didn't reveal any issues.
  5. Workaround we provide to customers changes current directory process-wide and no side-effects were reported.

@pakrym pakrym added the Servicing-consider Shiproom approval is required for the issue label Dec 27, 2018
@pakrym pakrym requested a review from jkotalik December 27, 2018 20:14
@natemcmaster
Copy link
Contributor

Needs to be rebased on release/2.2 code reorganization.

@natemcmaster natemcmaster added this to the 2.2.x milestone Jan 3, 2019
@natemcmaster
Copy link
Contributor

Please wait to merge until release/2.2 branches are open and this has been approved by shiproom. cref https://github.com/aspnet/AspNetCore-Internal/issues/1610

@vivmishra vivmishra modified the milestones: 2.2.x, 2.2.2 Jan 3, 2019
@vivmishra vivmishra added Servicing-approved Shiproom has approved the issue Servicing-consider Shiproom approval is required for the issue and removed Servicing-consider Shiproom approval is required for the issue Servicing-approved Shiproom has approved the issue labels Jan 3, 2019
@vivmishra vivmishra modified the milestones: 2.2.2, 2.2.x Jan 3, 2019
@vivmishra
Copy link

vivmishra commented Jan 3, 2019 •
edited

We should bring this back to Shiproom with ANCM folks present.

Fix the PR to segregate the test changes.

@jamshedd jamshedd added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Jan 8, 2019
@jamshedd
Copy link
Member

jamshedd commented Jan 8, 2019

@muratg The PR needs to be cleaned up. Cherry pick just the needed commit.

@jamshedd jamshedd modified the milestones: 2.2.x, 2.2.2 Jan 8, 2019
@muratg
Copy link
Contributor

muratg commented Jan 8, 2019

@pakrym This PR shouldn't have the file renames, keep that in master. And there are conflicts as seen above. Please do the needful and let me know.

@pakrym
Copy link
Contributor Author

pakrym commented Jan 8, 2019

@muratg release/2.2 was force pushed from under my PR. I'll remerge.

@jamshedd
Copy link
Member

jamshedd commented Jan 8, 2019

Approved for 2.2.2.

@KANekT KANekT mentioned this pull request Jan 14, 2019
Closed
@dougbu
Copy link
Member

dougbu commented Jan 14, 2019

Before merging, please update the 2.2.2 section of eng/PatchConfig.props: https://github.com/aspnet/Extensions/blob/d8407116d183debaadb801c0cbc41d65927999d6/eng/PatchConfig.props#L7-L10

@pakrym
Copy link
Contributor Author

pakrym commented Jan 14, 2019

Did it in #6483

@pakrym pakrym merged commit 5fd9435 into release/2.2 Jan 15, 2019
@natemcmaster natemcmaster deleted the pakrym/22-cd branch January 18, 2019 19:17
@czimbortibor czimbortibor mentioned this pull request Jul 23, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkotalik jkotalik jkotalik approved these changes

Assignees
No one assigned
Labels
Servicing-approved Shiproom has approved the issue
Projects
None yet
Milestone
2.2.2
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@pakrym @natemcmaster @vivmishra @jamshedd @muratg @dougbu @jkotalik