[jira] [Closed] (AMQ-1233) Bug when setting transportConnector URI options in activemq.xml

["Timothy Bish (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/AMQ-1233?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Timothy Bish closed AMQ-1233.
-----------------------------

    Resolution: Fixed

Appears to be fixed, reopen with test case if its still an issue.

> Bug when setting transportConnector URI options in activemq.xml
> ---------------------------------------------------------------
>
>                 Key: AMQ-1233
>                 URL: https://issues.apache.org/jira/browse/AMQ-1233
>             Project: ActiveMQ
>          Issue Type: Bug
>          Components: Broker
>    Affects Versions: 5.2.0
>            Reporter: David Martín Clavo
>             Fix For: NEEDS_REVIEWED
>
>
> As you may know, there are some problems when setting options in the URI for a TransportConnector in the broker configuration file activemq.xml (at least for me when launching the console with the Main class and the start command).
> For example, the option 'socket.tcpNoDelay=true' works fine in a producer or consumer URI, but doesn't make the broker have TCP_NODELAY sockets when it is set in the activemq.xml file.
> I think I have this more or less nailed down, I will try to explain.
> During all my explanation, I'm using the last source from SVN at the time of writing.
> Let's imagine you have the following piece of xml in your activemq.xml file:
>     <transportConnectors>
>       <transportConnector uri="tcp://localhost:61616?trace=true&foo=bar"/>
>     </transportConnectors>
> (side note: i have not found in the documentation the need to use ' & a m p ;' in this file, instead of just '&' like in Java code, would be a nice addition for clueless people like myself)
> (side note 2: in the forum the & a m p ; appears as just &, but you have to write '& a m p ;' without the spaces of course).
> How do these options get processed? Well at some point the method 'org.apache.activemq.transport.tcp.TcpTransportFactory.doBind' gets executed.
>     public TransportServer doBind(String brokerId, final URI location) throws IOException {
>         try {
>             Map options = new HashMap(URISupport.parseParamters(location));
> We now have a nice HashMap with our options: {trace=true, foo=bar}
>             ServerSocketFactory serverSocketFactory = createServerSocketFactory();
>             TcpTransportServer server = createTcpTransportServer(location, serverSocketFactory);
>             server.setWireFormatFactory(createWireFormatFactory(options));
>             IntrospectionSupport.setProperties(server, options);
> 'IntrospectionSupport.setProperties' is a method who uses reflexion to find the setter methods of a class, and if there is a setter method with the same name as a key of our HashMap, sets the value.
> The 'doBind' method finishes wish
>             Map transportOptions = IntrospectionSupport.extractProperties(options, "transport.");
>             server.setTransportOption(transportOptions);
>             server.bind();
>            
>             return server;
>         }
>         catch (URISyntaxException e) {
>             throw IOExceptionSupport.create(e);
>         }
>     }
> This is what happens when you start the broker.
> Now, what happens when a client connects and the broker has to create a Transport object for the connection?
> The method org.apache.activemq.transport.tcp.TcpTransportServer.run() gets executed:
>     [some stuff]
>                         HashMap options = new HashMap();
>                         options.put("maxInactivityDuration", new Long(maxInactivityDuration));
>                         options.put("minmumWireFormatVersion", new Integer(minmumWireFormatVersion));
>                         options.put("trace", new Boolean(trace));
>                         options.putAll(transportOptions);
>                         WireFormat format = wireFormatFactory.createWireFormat();
>                         Transport transport = createTransport(socket, format);
>                         Transport configuredTransport = transportFactory.serverConfigure(transport, format, options);
>                         getAcceptListener().onAccept(configuredTransport);
>     [more stuff]
> As you see this method constructs a new HashMap options object, which is used to configure the Transport with the line:
>                         Transport configuredTransport = transportFactory.serverConfigure(transport, format, options);
> If you have a look at this method, you will see that after a call to another method, the properties of the Transport are set again with 'IntrospectionSupport.setProperties'.
> So what's the problem? The problem is that for the properties to be recognized, they have to be declared as setters in 2 classes. In this case, TcpTransport and TcpTransportServer.
> When a property is set in the connection URI of a client, the ActiveMQ classes don't use TcpTransportServer. They directly set the options on the TcpTransport.
> However in a broker, the execution path goes through the TcpTransportServer class. If the options don't have setters declared, and are not later put into the 'options' object in the The method 'org.apache.activemq.transport.tcp.TcpTransportServer.run()' method, the Transport created by the broker does not have these options set.
> In our example, trace=true will be recognized but not foo=bar.
> If you have a look at the TcpTransportServer class, you will see that the only options accepted are:
> -maxInactivityDuration
> -minmumWireFormatVersion (small mispelling btw)
> -trace
> -transport.* options
> Furthermore, the way things work, if you put another option on the URI, it will be silently ignored. You can put 'foo=bar' or 'socket.tcpNoDelay=true' and no error message appears. So people may have been using options and thinking they had no effect, where in fact they were ignored without warning, like in the TCP_NODELAY case.
> I don't know how the ActiveMQ devs want to design this class, but I see 2 solutions:
> -manually add the missing options to TcpTransportServer, and update the docs so that they are synchronized with the actual options that have to be used (like socket.tcpNoDelay=true instead of wireFormat.tcpNoDelayEnabled=true).
> -somehow eliminate the double IntrospectionSupport.setProperties usage, and pass the options directly to the TcpTransport class. This will save the need to remember to put options in 2 classes every time one is added. And of course, there is still the need to keep the docs synchronized... :)
> If you want to reproduce my analysis, I used Eclipse, executing ActiveMQ source using the console Main class, in debug mode, and using breakpoints in the methods above mentioned.

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira