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.

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

Why System.Net.Sockets has dependency to System.Security.Principal.Windows? #1383

Closed
dehghani-mehdi opened this issue Dec 10, 2019 · 8 comments · Fixed by #52711
Closed

Why System.Net.Sockets has dependency to System.Security.Principal.Windows? #1383

dehghani-mehdi opened this issue Dec 10, 2019 · 8 comments · Fixed by #52711

Comments

@dehghani-mehdi
Copy link

Why System.Runtime.Caching has dependency to System.Security.Principal.Windows?

Okay, the comment should be

Why System.Net.Sockets has dependency to System.Security.Principal.Windows?

to match new title.

@stephentoub
Copy link
Member

@dehghani-mehdi, why did you open a new issue here? https://github.com/dotnet/corefx/issues/42592 had been tracking your previous same question, and it was answered there. You also replied to that one saying it could be closed (though your response appears to have been deleted).

@dehghani-mehdi
Copy link
Author

dehghani-mehdi commented Dec 10, 2019

Because it's not and that was not a question, it's a bug (maybe I should change the title to something like System.Runtime.Caching should not has dependency to System.Security.Principal.Windows). you wrote some technical reasons there, for me as user the chat is not matter, the issue is why memory cache has dependency to some unrelated package and why I have to include some un-related dll in my project.

though your response appears to have been deleted

Yes, because you closed the issue.

@stephentoub
Copy link
Member

stephentoub commented Dec 10, 2019

Because it's not

Maybe it was a misunderstanding. @StephenBonikowsky asked "do we need to keep this issue open?" and you responded "I don't think so". But maybe you were answering a different question.

Yes, because you closed the issue.

I closed the issue because you commented that the issue needn't be kept open. It sounds like we had a miscommunication.

it's a bug

What's the bug? Are you saying that something is breaking at runtime? Or you just don't want this extra .dll deployed as part of your app? @vcsjones outlined nicely in https://github.com/dotnet/corefx/issues/42592#issuecomment-553989245 why this dependency is there.

@dehghani-mehdi
Copy link
Author

dehghani-mehdi commented Dec 10, 2019

I closed the issue because you commented that the issue needn't be kept open

I meant the question and the issue was not answered solved. but that comment. for the record, the comment was: 'I don't think so'

It sounds like we had a miscommunication.

Yes, it seems like we had. sorry I didn't explain well.

Are you saying that something is breaking at runtime?

No, everything works great

Or you just don't want this extra .dll deployed as part of your app?

Yes, I am developer too, it is clear (at least to me) memory cache has nothing to do with System.Security.Principal.Windows, I'm Windows user, but the name Windows here is another problem, It seems like hard-coded value to me.
But my main issue is the extra dll.

@StephenMolloy
Copy link
Member

I don't believe this is a System.Runtime.Caching bug at this point. The cache package relies on ConfigurationManager, and that is expected. The issue that remains is that System.Security.Principal.Windows does not get linked away as expected, because of System.Net.Sockets? https://github.com/dotnet/corefx/issues/42592#issuecomment-555680784

@danmoseley danmoseley changed the title Why System.Runtime.Caching has dependency to System.Security.Principal.Windows? Why System.Net.Sockets has dependency to System.Security.Principal.Windows? Dec 18, 2019
@danmoseley
Copy link
Member

Updating title to match my reading of that comment.

@StephenBonikowsky StephenBonikowsky transferred this issue from dotnet/corefx Jan 7, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Security untriaged New issue has not been triaged by the area owner labels Jan 7, 2020
@StephenBonikowsky StephenBonikowsky removed area-System.Security untriaged New issue has not been triaged by the area owner labels Jan 7, 2020
@StephenBonikowsky
Copy link
Member

StephenBonikowsky commented Jan 24, 2020

@danmosemsft
If I understand this correctly, the necessary fix should be done in the tool that analyzes an app and is supposed to link away unneeded assemblies.

What area label should be applied to this issue?

@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@StephenBonikowsky StephenBonikowsky removed the untriaged New issue has not been triaged by the area owner label Feb 24, 2020
@karelz karelz added this to the Future milestone Jul 30, 2020
@geoffkizer
Copy link
Contributor

The reason that System.Net.Sockets has a dependency on System.Security.Principal.Windows is because the old APM-style async methods are implemented using ContextAwareResult, which supports flowing identity and thus requires System.Security.Principal.Windows.

The System.Net.Sockets code isn't actually using ContextAwareResult to flow identity, as far as I can see. So it's never used in practice.

Longer-term, we should reimplement the old APM style async stuff on top of the new Task based methods; this will eliminate the dependency on ContextAwareResult and should provide a bunch of other benefits too, like improving perf.

Short-term, we could look at refactoring the existing dependency. I'm not sure why the Sockets code uses ContextAwareResult here instead of LazyAsyncResult, but there's presumably a good reason for it.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 13, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 14, 2021
@karelz karelz modified the milestones: Future, 6.0.0 May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants