rebus-org / Rebus.SqlServer

:bus: Microsoft SQL Server transport and persistence for Rebus
https://mookid.dk/category/rebus
Other
43 stars 42 forks source link

ExpiredMessagesCleanup runs even for a OneWayClient #28

Closed MrMDavidson closed 5 years ago

MrMDavidson commented 6 years ago

This is possibly more of a design decision but we have some roles which run as a oneway client;

RebusConfigurer config;
config.Transport(x => {
  x.Register(res => new SqlServerLeaseTransport(...);
  OneWayClientBackdoor.ConfigureOnewayClient(x);
});

We've noticed in our logs that even in this scenario that the transport still runs its internal task ExpiredMessagesCleanup. To me it seems like that if the transport is one way then it should only ever put messages into the queue. Not do any other work.

mookid8000 commented 6 years ago

To me it seems like that if the transport is one way then it should only ever put messages into the queue. Not do any other work.

You are absolutely right 😄

MrMDavidson commented 6 years ago

Phew! I imagine this might be wide spread across other transports, too, in that case. For reference we're on 4.0.0 of Rebus and 5.0.0-b5 of Rebus.Sqlserver.

mookid8000 commented 6 years ago

I think this is special for the SQL transport (and maybe PostgreSQL and Oracle too, as I believe they were pretty much based on the SQL transport).

The task is responsible for removing rows from the table when the time-to-be-received has expired.

Some other transports have this mechanism built in, and with RabbitMQ Rebus simply discards expired messages upon receiving them.

mookid8000 commented 5 years ago

Hmm, I cannot figure out how the expired messages cleanup task can end up running for a one-way client.

A one-way client has a null input queue name in the transport, so the Initialize method, which normally starts the cleanup task, doesn't start it:

public void Initialize()
{
    if (ReceiveTableName == null) return;

    _expiredMessagesCleanupTask.Start();
}

From the code you posted it seems you are not using "the official" configuration extension

Configure.With(...)
    .Transport(t => t.UseSqlServerInLeaseMode(...))
    .(...)
    .Start();

but rather it seems you are registering the transport implementation directly. Could you show me some more of that code?

MrMDavidson commented 5 years ago

Ahh, yes, correct. At the time we integrated with Rebus the extensions had behaviour we didn't want (it was either automatic table creation - we have specific migrations for that or the specific database connection provider / factory. The code comment implies the former but I have a recollection of the latter being a factor too).

We are definitely passing the input name into our construction of the transport;

// Note: We do not use the StoreInSql() convenience method as it does not provide a way to not-create tables
config.Transport(
    x => {
        x.Register(res => new SqlServerLeaseTransport(new DbConnectionFactoryDbConnectionProvider(dbConnectionFactory, res.Get<IRebusLoggerFactory>()), "RebusMessage", inputQueueName, res.Get<IRebusLoggerFactory>(), res.Get<IAsyncTaskFactory>(), TimeSpan.FromMinutes(3), TimeSpan.FromSeconds(30), () => machineName, TimeSpan.FromMinutes(2)));

        if (processMessages == false) {
            OneWayClientBackdoor.ConfigureOneWayClient(x);
        }
    }
);

I'll do some testing to see if not passing in the queue name to the transport resolves it / has any other issues.

I wonder if OneWayClientBackdoor could throw if the transport is misconfigured in such a way that it can't be a one way client?

mookid8000 commented 5 years ago

We are definitely passing the input name into our construction of the transport;

So that's why it starts the background task – the transport thinks it's regular 😁

And yes, I guess OneWayClientBackdoor should throw if the transport's input queue was set to something ... I'll see if I can make that happen.