jbogard / MediatR

Simple, unambitious mediator implementation in .NET
Apache License 2.0
11k stars 1.16k forks source link

Fixed missing CancellationToken usage in MediatR class registration #1054

Closed samanazadi1996 closed 1 month ago

samanazadi1996 commented 1 month ago
zachpainter77 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

@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