jhalterman / lyra

High availability RabbitMQ client
Apache License 2.0
263 stars 74 forks source link

Why to copy ConnectionFactory on each create #73

Open igreenfield opened 7 years ago

igreenfield commented 7 years ago

I see in class Connections lines 108 - 118:

public static ConfigurableConnection create(ConnectionOptions options, Config config, ClassLoader classLoader)
      throws IOException, TimeoutException {
    Assert.notNull(options, "options");
    Assert.notNull(config, "config");
    Assert.notNull(classLoader, CLASS_LOADER_PARAMETER_NAME);
    ConnectionHandler handler = new ConnectionHandler(options.copy(), new Config(config), classLoader);
    ConfigurableConnection proxy = (ConfigurableConnection) Proxy.newProxyInstance(
        classLoader, CONNECTION_TYPES, handler);
    handler.createConnection(proxy);
    return proxy;
  }

Why on line 113 you copy the options? or inside this class copy the ConnectionFactory and not reuse it?

igreenfield commented 7 years ago

Hi @jhalterman do you know why the lib always copy? hence when we use new version of the factory it may have new properties and if we forget to copy this it may be problem....

jhalterman commented 7 years ago

@igreenfield The thinking was that options are things that help you create a Connection, which cannot be changed once the connection is made, and Config are things that can be changed after the Connection is made. A new version of a connection factory should require a new connection, right?

igreenfield commented 7 years ago

@jhalterman New Connection yes, but hear you create connection from options the user pass to you... so why you copy it first? even if you do copy do you need deep copy?

jhalterman commented 7 years ago

@igreenfield The thinking was I didn't want changes to the Options object to effect future connection attempts which is why we make a copy.

What sort of use case do you have where this is a problem?

igreenfield commented 7 years ago

@jhalterman The 'ConnectionOptions' object expose the factory so as user I can change it and I will expect that my changes will take effect but with the copy it won't happen...

jhalterman commented 7 years ago

@igreenfield That's true. You would prefer ConnectionFactory changes to take effect if the connection is ever lost and recovered? In that case, why not make a new connection? As for the current behavior, do you think this could use more clarification in the docs?

igreenfield commented 7 years ago

@jhalterman If I use the withConnectionFactory option and pass in my factory I would think the lib will use it but in the current implementation the lib will copy part of it and use other instance... this is not so good...