BrighterCommand / BrighterCommand/Brighter
SqsMessageProducer casts Scheduler without a guard (V3 and V4) - a delayed send over 15 minutes on a Standard queue throws NullReferenceException or InvalidCastException
- Dominant language
- C#
- Stars
- 2.5k
- Forks
- 296
- Avg merge
- 1d 11h
- Merged PRs (30d)
- 21
Description
`SqsMessageProducer.SendWithDelayAsync` casts `Scheduler` without a guard, in both V3 and V4. A delayed send over 15 minutes on a Standard queue throws `NullReferenceException` or `InvalidCastException` from inside the send, naming nothing the operator can act on.
This is the same defect fixed for `SnsMessageProducer` in `f5cc31e12`. **The difference that makes this one more urgent: SQS already passes `delay` through on `master`, so both arms are reachable today.** The SNS sync arm only became reachable on the #4297 branch.
## Where
- `src/Paramore.Brighter.MessagingGateway.AWSSQS/SqsMessageProducer.cs:150` and `:155`
- `src/Paramore.Brighter.MessagingGateway.AWSSQS.V4/SqsMessageProducer.cs:150` and `:155`
Both files are identical in this region and neither is touched by #4297.
```csharp
// SQS support delay until 15min, more than that we are going to use scheduler
if (delay > TimeSpan.FromMinutes(15) && _publication.QueueAttributes.Type == SqsType.Standard)
{
if (useAsyncScheduler)
{
var schedulerAsync = (IAmAMessageSchedulerAsync)Scheduler!; // <-- unguarded
await schedulerAsync.ScheduleAsync(message, delay.Value, cancellationToken);
return;
}
var schedulerSync = (IAmAMessageSchedulerSync)Scheduler!; // <-- unguarded
schedulerSync.Schedule(message, delay.Value);
return;
}
```
## Reaching it
Both conditions have to hold, which is why it has gone unnoticed:
1. `delay > TimeSpan.FromMinutes(15)` — under 15 minutes SQS handles the delay natively and the scheduler is never consulted.
2. `_publication.QueueAttributes.Type == SqsType.Standard` — a FIFO queue takes the native path.
Then:
| `Scheduler` | result |
|---|---|
| not configured (`null`) | `NullReferenceException` |
| implements only the *other* half of the sync/async pair | `InvalidCastException` |
`SendWithDelay` reaches the sync arm through `BrighterAsyncContext.Run(() => SendWithDelayAsync(message, delay, false))` at `:180`, so a synchronous caller hits `:155` and an asynchronous one hits `:150`.
Neither exception names `MessageSchedulerFactory`, which is the setting the operator actually has to change, and both surface from inside a send rather than at configuration time.
## Expected
The shape `f5cc31e12` applied to SNS, and that `RedisMessageProducer`, Kafka, MsSql, MQTT and the in-memory reference already use: prefer the half of the pair matching the call, accept the other, otherwise throw `ConfigurationException` naming `MessageSchedulerFactory`.
```csharp
if (useAsyncScheduler)
{
if (Scheduler is IAmAMessageSchedulerAsync schedulerAsync)
{
await schedulerAsync.ScheduleAsync(message, delay.Value, cancellationToken);
return;
}
}
// ... prefer-then-accept, else ConfigurationException naming MessageSchedulerFactory
```
Accepting the other half matters: a host that configures only one of the two should not have a delayed send fail on a cast.
## Notes
- Raised during the #4297 review and recorded there as a follow-up rather than fixed in-branch, because `SqsMessageProducer` is outside that PR's scope.
- The V3 and V4 copies should move together, as they did for SNS.
- Worth a test per arm per version (4 total), matching the 4 SNS tests in `f5cc31e12`.
Contributor guide
Assessment
This issue has not been assessed yet.