Asankha, I started to review the code and here are the most relevant questions:
1. The JMS Javadocs specify the thread safety characteristics of the Session object as follows: "A Session object is a single-threaded context for producing and consuming messages. [...] Once a connection has been started, any session with one or more registered message listeners is dedicated to the thread of control that delivers messages to it. It is erroneous for client code to use this session or any of its constituent objects from another thread of control. [...] The close method is the only session method that can be called while some other session method is being executed in another thread." If I understand the code correctly, if the cache level is CACHE_SESSION a single session is shared among all MessageListenerTasks, and if the cache level is CACHE_CONSUMER then also the MessageConsumer is shared. Therefore sessions and consumers are used concurrently by several threads, which I believe is in violation of the JMS specs. Also note that the effect of the corresponding cache levels in Spring's DefaultMessageListenerContainer is completely different: If the cache level is CACHE_SESSION, the session is cached at the AsyncMessageListenerInvoker level (the equivalent of the MessageListenerTask), i.e. each thread executes subsequent receives on the same session, but each thread has its own session. If the cache level is lower, a new session is created before every receive. According to Spring's Javadocs this is necessary with some containers to guarantee proper handling of JTA transactions. Note that issues caused by concurrent usage of the session object will probably not be detected by the current test suite because all tests except MinConcurrency only send a single message, and MinConcurrency blocks all the receiving threads until the expected level of concurrency (i.e. number of blocked threads) is reached. What we need here is a test that sends a burst of messages to test the stability of the receiver under load. This would also be interesting for the other transports. 2. As I explained in SYNAPSE-439, the old code implicitly allowed using a single connection factory for both queue and topic connections (if the underlying connection factory implements both QueueConnectionFactory and TopicConnectionFactory). This is basically the JMS 1.1 scenario (though nothing would prevent a JMS provider to use JMS 1.0 while still implementing both interfaces!). Your code now explicitly supports JMS 1.1, which is a good thing. However this also increases the clutter in the code (caused by the if-else statements and the flags and destination types that need to be passed around). When thinking about SYNAPSE-439 I came to the conclusion that the best solution for this particular problem would probably be to hide the differences between JMS 1.0/queue, JMS 1.0/topic and JMS 1.1 behind a thin abstraction layer on top of JMS. The idea would be to define a set of interfaces that would basically be a subset of the JMS 1.1 API (more precisely the part of the API that is independent of the destination type), and to have three implementations per interface. E.g. one would have a JMSSession interface with JMSQueueSession, JMSTopicSession and JMS11Session implementations. I think that using this type of abstraction would make the code more readable, while causing a negligible overhead. It would also have some additional benefits: * If the abstraction layer doesn't give access to the underlying JMS objects, this is an easy way to make sure that nobody calls JMS 1.1 specific methods accidentally. * It is object oriented (which is not the case for the existing methods in JMSUtils that are used to hide the API differences). * In the future this abstraction layer could be extended to support provider specific extensions or optimizations, while still keeping the core code clean. I also believe that at some point we will see a common code base for the JMS and QPID transports (as I proposed some time ago in SYNAPSE-303) and this kind of abstraction could be the first step in that direction. 3. The JMS transport only supports scenarios where a destination is statically bound to a service. This is basically the logic supported by AbstractTransportListener. I believe that there will be requirements to support other scenarios, in particular the possibility to listen on a JMS destination but to let Axis2 dispatch to the appropriate service. AXIS2-1665 requests exactly this type of enhancement. Actually all the current transports fall into one of two categories: * The AbstractTransportListener based transports bind protocol endpoints statically to services. * The others only allow to listen on a single protocol endpoint. Here, by "protocol endpoint" I mean TCP port, mail account, JMS destination, file/directory etc. Actually I changed AbstractPollingTransportListener some time ago so that it supports both scenarios (as well as the mixed scenario). I believe that most of the transports (with the notable exception of HTTP) should support this and that in the longer term the required infrastructure should be built into AbstractTransportListener. I'm not saying that we should refactor the JMS transport now, but I think we should have an architecture that will eventually support this. This was one of the reasons why I introduced the JMSEndpoint class. Indeed a prerequisite to support the above requirement is that the transport no longer relies on the assumption that the configuration comes from an AxisService object. Instead all the protocol endpoint configuration should be encapsulated in JMSEndpoint (Interestingly classes that play the same role for some of the other transports already exist, such as AbstractPollTableEntry and subclasses, as well as DatagramEndpoint and subclasses). Also the code that builds the JMSEndpoint object should be easily refactorable so that it refers to ParameterInclude (see AbstractPollingTransportListener#createPollTableEntry) or even a simple Map instead of AxisService. I'm not sure yet what would be the impact of this enhancement on the new JMS transport implementation and vice-versa. What do you think about this? Andreas On Sat, Nov 15, 2008 at 17:11, Asankha C. Perera <[EMAIL PROTECTED]> wrote: > Hi Andreas >> >> Can you commit it to a private branch? That makes it easy to see both >> the differences with respect to the existing code as well as incremental >> changes. >> > > Checked into : > https://svn.apache.org/repos/asf//webservices/commons/trunk/scratch/asankha/transport/ > > asankha > > -- > Asankha C. Perera > http://adroitlogic.org > > http://esbmagic.blogspot.com > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [EMAIL PROTECTED] > For additional commands, e-mail: [EMAIL PROTECTED] > > --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
