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

Static logging delegates nested inside generic types #31340

Closed
JamesNK opened this issue Mar 29, 2021 · 7 comments
Closed

Static logging delegates nested inside generic types #31340

JamesNK opened this issue Mar 29, 2021 · 7 comments
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions help wanted Up for grabs. We would accept a PR to help resolve this issue task
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Mar 29, 2021

Static logging delegates will allocate a new copy for each generic type.

See #31333 (comment)

Should audit for all usages of static logging delegates to see if there are other places they are nested in generic types.

Before:

// RequestContext.Log.cs
internal sealed partial class RequestContext<TContext>
{
    private static class Log { }
}

After:

// RequestContext.Log.cs
internal static class RequestContext
{
    private static class Log { }
}

Edit: That won't work because Log will be private. How about internal static class RequestContextLog with no nested Log type.

@JamesNK JamesNK added area-servers help wanted Up for grabs. We would accept a PR to help resolve this issue labels Mar 29, 2021
@andrzejpindor
Copy link
Contributor

Happy to pick this up - in #31345 I've already refactored RequestContext<T> but I can also check other places where situation may be similar.

@martincostello
Copy link
Member

Other than the two already mentioned in #31333, I also found these two which I can fix up at the same time:

public class HubConnectionHandler<THub> : ConnectionHandler where THub : Hub

internal partial class DefaultHubDispatcher<THub>
{
private static class Log

@martincostello
Copy link
Member

Oh - I see you've already done it in #31345 - I was already in the middle of doing it from this comment #31333 (comment), but happy for you to do it instead.

@andrzejpindor
Copy link
Contributor

I can also cover the two cases mentioned by you and scan project for similar problems. I'll wait to become an assignee to this issue to continue work ;)

@martincostello
Copy link
Member

I did a search in VS Code across the whole ASP.NET Core solution for private static class Log and those were the only other two I could find that were inside a generic type.

I hadn't started on changing those two yet, so feel free to make the change yourself 🙂

@jkotalik
Copy link
Contributor

Feel free to pick up the ones in SignalR as well.

@BrennanConroy
Copy link
Member

Done via #31364

@ghost ghost locked as resolved and limited conversation to collaborators Apr 28, 2021
@danroth27 danroth27 added the task label May 14, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions help wanted Up for grabs. We would accept a PR to help resolve this issue task
Projects
None yet
Development

No branches or pull requests

7 participants