reactor / reactor-rabbitmq

Reactor RabbitMQ
Apache License 2.0
157 stars 55 forks source link

Using senderOptions.connectionSupplier results in multiple connections created by the Sender #93

Closed philsttr closed 5 years ago

philsttr commented 5 years ago

If I specify a senderOptions.connectionSupplier in order to provide a connection name, then the Sender is opening a new connection for each new channel, instead of reusing the same connection for each channel. When I do not specify a connectionSupplier, only one connection is opened.

If a connectionSupplier is not specified, then the Sender constructs a custom connectionMono that caches the connection result (meaning that the same connection will always be used for each subscription). See here.

If a connectionSupplier is specified, the senderOptions.connectionMono is non-null (and not cached). Therefore a new connection is created for each subscription. I believe this means that a new connection is created for every channel here

Ideally, I would like the ability to specify a connection name via a channelSupplier, and still only have one connection created, instead of one per channel)

In addition, I'm a little concerned about not having the other default behavior that Sender adds to the connectionMono when specifying a connectionSupplier.

acogoluegnes commented 5 years ago

The client-provided Mono<Connection> should handle the caching if this a feature it wants. You noticed the sender closes the connection only if it created the connection itself, as it assumes the client should close a Mono<Connection> it provided (maybe this connection is used somewhere else and should survive the sender instance).

I think I get the idea of being able to choose the connection to use based on a name, on channel creation, but I'm not sure I understand how the name to use would be picked up (as channel creation happens inside the sender and not in client code).

philsttr commented 5 years ago

Let me clarify my use case.

I want to be able to provide a custom connection name. The easiest way to do that currently is to provide a custom connectionSupplier...

senderOptions.connectionSupplier(
    connectionFactory -> connectionFactory.newConnection("myCustomName"));

I noticed that after I did that, I "lost" the default behavior that the Receiver provides. Specifically, I lost:

One of the side-effects of losing this behavior, is that a new connection is opened for each channel (since caching was lost).

I feel like if only connectionSupplier is specified (without specifying a connectionMono), then that connectionSupplier should be used in Mono.fromCallable here, while keeping the other default behaivor. Otherwise, using a connectionSupplier seems dangerous at the moment.

In my case, I have worked around this problem by providing a custom connectionMono instead of a connectionSupplier, since I needed to manage the connectionSubscriptionScheduler and caching as well (and also to work around #94).

At a minimum, usage of connectionSupplier should be documented with the effects that are lost.

acogoluegnes commented 5 years ago

This makes sense and can be fixed by propagating the connection supplier to the sender constructor like you suggest. Documentation should be clarified as well.