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

Fixed missing CancellationToken usage in MediatR class registration #1054

Closed
wants to merge 1 commit into from

Conversation

samanazadi1996
Copy link

  • Added missing CancellationToken parameter to ConnectImplementationsToTypesClosing method calls.

@zachpainter77
Copy link
Contributor

zachpainter77 commented Aug 3, 2024

Just curious.. what does adding the cancellation tokens to the calls change? The methods already have a cancellation token passed to them.. they all get the default(CancellationToken) value.

image

I guess what I'm asking is... What exactly are you "fixing"?

@samanazadi1996
Copy link
Author

@zachpainter77
Currently, the 'AddMediatR' method allows users to customize timeout and configuration settings for MediatR. When users specify the 'RegistrationTimeout', it is expected that the relevant methods will adhere to these settings to manage system performance effectively and handle cancellation requests as needed.

@zachpainter77
Copy link
Contributor

zachpainter77 commented Aug 3, 2024

@zachpainter77 Currently, the 'AddMediatR' method allows users to customize timeout and configuration settings for MediatR. When users specify the 'RegistrationTimeout', it is expected that the relevant methods will adhere to these settings to manage system performance effectively and handle cancellation requests as needed.

The only relevant methods are the ones that already have the cancellation token passed. Your PR doesn't do anything that is valuable. The only thing that makes use of the timeout are registrations of open generic IRequestHandler implementations. Nothing else will ever make use of the timeout configuration.

There are only two methods that call cancellationToken.ThrowIfCancellationRequested(); and those are AddAllConcretionsThatClose and GenerateCombinations. Those methods are only called when there are open generic implementations of IRequestHandler.

I know this because I authored the feature (#1048). I am the one who wrote the code to add the timeout configuration. It is only being used by the generics registration feature. If you are wishing to change this to work for all registrations globally then you will need to do more than pass the cancellation token to the method calls. You will need to add more places where the cancellationToken.ThrowIfCancellationRequested(); is called as well.

An open generic example:

public class MyOpenGenericRequest<TEntity, TDto> : IRequest<TDto>
    where TEntity : class, IEntity,
    where TDto : class, IDto, new()
{
    public int EntityId { get; set; }
}

public class MyOpenGenericRequestHandler<TEntity, TDto> : IRequestHandler<MyOpenGenericRequest<TEntity, TDto>, TDto>
    where TEntity : class, IEntity,
    where TDto : class IDto
{
    private IRepository<TEntity> _repo;
    private IMapper _mapper;

    public MyOpenGenericRequestHandler(IRepository<TEntity> repo, IMapper mapper)
    {
        _repo = repo;
        _mapper = mapper;
    }

    public async Task<TDto> Handle(MyOpenGenericRequest<TEntity, TDto> request, CancellationToken cancellationToken)
    {
        var entity = await _repo.FindAsync(request.EntityId);
        
        return _mapper.Map<TDto>(entity);
    }
}   

@samanazadi1996
Copy link
Author

@zachpainter77 Currently, the 'AddMediatR' method allows users to customize timeout and configuration settings for MediatR. When users specify the 'RegistrationTimeout', it is expected that the relevant methods will adhere to these settings to manage system performance effectively and handle cancellation requests as needed.

The only relevant methods are the ones that already have the cancellation token passed. Your PR doesn't do anything that is valuable. The only thing that makes use of the timeout are registrations of open generic IRequestHandler implementations. Nothing else will ever make use of the timeout configuration.

There are only two methods that call cancellationToken.ThrowIfCancellationRequested(); and those are AddAllConcretionsThatClose and GenerateCombinations. Those methods are only called when there are open generic implementations of IRequestHandler.

I know this because I authored the feature (#1048). I am the one who wrote the code to add the timeout configuration. It is only being used by the generics registration feature. If you are wishing to change this to work for all registrations globally then you will need to do more than pass the cancellation token to the method calls. You will need to add more places where the cancellationToken.ThrowIfCancellationRequested(); is called as well.

An open generic example:

public class MyOpenGenericRequest<TEntity, TDto> : IRequest<TDto>
    where TEntity : class, IEntity,
    where TDto : class, IDto, new()
{
    public int EntityId { get; set; }
}

public class MyOpenGenericRequestHandler<TEntity, TDto> : IRequestHandler<MyOpenGenericRequest<TEntity, TDto>, TDto>
    where TEntity : class, IEntity,
    where TDto : class IDto
{
    private IRepository<TEntity> _repo;
    private IMapper _mapper;

    public MyOpenGenericRequestHandler(IRepository<TEntity> repo, IMapper mapper)
    {
        _repo = repo;
        _mapper = mapper;
    }

    public async Task<TDto> Handle(MyOpenGenericRequest<TEntity, TDto> request, CancellationToken cancellationToken)
    {
        var entity = await _repo.FindAsync(request.EntityId);
        
        return _mapper.Map<TDto>(entity);
    }
}   

Thank you for your explanation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants