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

Do we need IAvoidDuplicateCrossCuttingConcerns #15538

Closed
ahmetfarukulu opened this issue Jan 29, 2023 · 9 comments
Closed

Do we need IAvoidDuplicateCrossCuttingConcerns #15538

ahmetfarukulu opened this issue Jan 29, 2023 · 9 comments
Assignees
Labels

Comments

@ahmetfarukulu
Copy link
Member

Description

As far as I understand it controllers/pages don't use interceptions for performance issue but we're adding some unnecessary check

Analysis

As you notice apply method take object as parameter to avoid duplicate CCC and we give controller/page for that also we only use with filters but filters object(Controller/Page) never intercept by AbpInterception

@maliming maliming self-assigned this Jan 30, 2023
@maliming maliming removed the problem label Jan 30, 2023
@maliming
Copy link
Member

hi

We will try to ignore the Interceptors if there is a filter applied.

https://github.com/abpframework/abp/search?l=C%23&q=IsApplied

@ahmetfarukulu
Copy link
Member Author

Hi,

We only apply for action/page filters and apply object is controller/page but we don't use interception for this types

In this issue reason for apply to not check twice but we can use AsyncLocal

public class NotifyOnExceptionInterceptor : AbpInterceptor, ITransientDependency
{
    private static readonly AsyncLocal<bool> IsExecute = new AsyncLocal<bool>();

    public NotifyOnExceptionInterceptor()
    {

    }

    public override async Task InterceptAsync(IAbpMethodInvocation invocation)
    {
        if (IsExecute.Value)
        {
            await invocation.ProceedAsync();
            return;
        }

        try
        {
            IsExecute.Value = true;
            await invocation.ProceedAsync();
        }
        catch (Exception e)
        {
            //Log exception
            throw;
        }
    }
}

@maliming
Copy link
Member

maliming commented Feb 2, 2023

private static readonly AsyncLocal<bool> IsExecute = new AsyncLocal<bool>();

I think this won't work. Did you try that?

@ahmetfarukulu
Copy link
Member Author

It works important thing is interception start with Method1Async with this way every sub methods include the same context

image

@maliming
Copy link
Member

maliming commented Feb 2, 2023

Does it also work for multiple http requests?

@ahmetfarukulu
Copy link
Member Author

Yes every http requests different scope

@maliming
Copy link
Member

maliming commented Feb 2, 2023

hi

Can you open a pr? then I will test it. Thanks

@ahmetfarukulu
Copy link
Member Author

ahmetfarukulu commented Feb 5, 2023

Hi @maliming

Filter avoid object and interception target object is different that's why IAvoidDuplicateCrossCuttingConcerns not work as expected. It check twice therefore these usings are unnecessary.

I think there is some confusion here BlogAdminAppService and BlogAdminController use same attributes(Authorize, RequiresFeature) in that case it check twice first action filter check after interception check(interception target object use BlogAdminAppService and action filter avoid object use BlogAdminController)

If we want to check once then it must be like TenantAppService and TenantController(Authorize check only TenantAppService)

In conclusion i think we can remove IAvoidDuplicateCrossCuttingConcerns and related things. Ofcourse this is not a complete solution it still check twice for prevent this we can check only on appservice side

@stale
Copy link

stale bot commented Apr 7, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the inactive label Apr 7, 2023
@stale stale bot closed this as completed May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants