PaulGale commented on issue #2271:
URL: https://github.com/apache/activemq/issues/2271#issuecomment-5147123712

   @mattrpav — direct answer to your question: **ALL sends are broken**, not 
only ones that specify a delivery delay.                                        
                                                                                
                                     
                                                                                
                                                                                
                                                                                
                                    
     The key detail is that nothing *sets* `deliveryDelay`. Neither application 
code nor pooled-jms does. pooled-jms **reads** it, once per producer 
construction, to snapshot the producer's default state so it can be restored 
when that pooled producer is handed to the next borrower. That is ordinary 
pooling hygiene rather than delivery-delay usage.                               
                                                                                
                                                                                
    
                                                                                
                                                                                
                                                                                
                                    
     Crucially, pooled-jms does not call the JMS 2.0 getter blindly. It guards 
the call behind the provider's own capability answer, in 
`JmsPoolMessageProducer.<init>`:                                                
                                                            
                                                                                
                                                                                
                                                                                
                                    
     ```                                                                        
                                                                                
                                                                                
                                    
     101: iconst_2                                   // major = 2               
                                                                                
                                                                                
                                    
     102: iconst_0                                   // minor = 0               
                                                                                
                                                                                
                                    
     103: invokevirtual isJMSVersionSupported:(II)Z  // does the provider 
support JMS 2.0?                                                                
                                                                                
                                          
     106: ifeq          119                          // NO  -> skip, return     
                                                                                
                                                                                
                                    
     109: getDeliveryDelay()                         // YES -> call it          
                                                                                
                                                                                
                                    
     119: return                                                                
                                                                                
                                                                                
                                    
     ```                                                                        
                                                                                
                                                                                
                                    
                                                                                
                                                                                
                                                                                
                                    
     `isJMSVersionSupported` resolves from `connection.getMetaData()`. So 
against 6.2.8 the provider answers major=1, the guard is false, and the call 
never happens. Against 6.3.0 it answers major=3, the guard passes, 
`getDeliveryDelay()` throws, and every producer creation fails. So I would put 
this slightly differently than "a bug in the messaginghub pool": pooled-jms 
asks before calling an optional API, which is the right thing to do. What 
changed is the answer it gets.                                                  
                   
                                                                                
                                                                                
                                                                                
                                    
     That said, I do agree pooled-jms should be more defensive here. Calling an 
optional JMS 2.0 accessor from a constructor without catching 
`UnsupportedOperationException` is fragile regardless of what any provider 
advertises, and I am happy to raise that with them in parallel. I suspect the 
ideal outcome is a change in both projects.                                     
                                                                                
                                                                                
       
                                                                                
                                                                                
                                                                                
                                    
     Two corrections on the suggested workarounds, both of which I think matter 
for prioritisation:                                                             
                                                                                
                                    
                                                                                
                                                                                
                                                                                
                                    
     - **This is already the JMS 1.1 path.** The failing frame is 
`ActiveMQMessageProducerSupport`, the classic `MessageProducer` base class, 
reached via `JmsPoolSession.createProducer` from 
`JmsTemplate.doCreateProducer`. The `JMSContext`/`JMSProducer` implementation 
is a separate class (`ActiveMQProducer`). Avoiding `JMSContext` and staying on 
JMS 1.1 operations therefore does not avoid this.                               
                                                                                
                                     
                                                                                
                                                                                
                                                                                
                                    
     - **`activemq-pool` is not a Spring Boot fallback.** 
`spring-boot-dependencies` manages `org.messaginghub:pooled-jms` and only that. 
Neither `activemq-pool` nor `activemq-jms-pool` is managed by Boot, and neither 
is the auto-configured pooling path. They can still be wired manually, but 
there is no supported Spring Boot configuration that routes around this.        
                                                                                
                                                                                
           
                                                                                
                                                                                
                                                                                
                                    
     On sequencing, one detail from your own description of #2229 seems 
decisive: the delivery delay support there depends on **OpenWire protocol 
changes (v13)**. A wire-protocol bump cannot ship in a 6.3.x patch release, and 
#2229 is currently WIP against                    
     `activemq-wip-jakarta-3.1` with full 3.1 targeted at 6.5.0. So "wait for 
the real implementation" and "unbreak 6.3.x users" are not the same timeline.   
                                                                                
                                      
                                                                                
                                                                                
                                                                                
                                    
     That is really why I think a small interim change is worth considering for 
6.3.1, independent of #2229: have `getDeliveryDelay()` return `0` (the JMS 
default, meaning no delay, which a provider supporting only immediate delivery 
can report truthfully), accept            
     `setDeliveryDelay(0)` as a no-op, and continue to throw for a non-zero 
delay so a genuinely requested delay is never silently dropped. That needs no 
protocol change, keeps the 3.1 metadata claim that #1718 wanted, and fixes the 
whole class of capability-probing consumers rather than pooled-jms alone.       
                                                                                
                                                                                
                                                                           
                                                                                
                                                                                
                                                                                
                                    
     Fully understood that the 6.3.0 metadata change was a deliberate call to 
unblock the Jetty and Spring dependency set. That unblocking is genuinely 
valuable, and it is the reason I wanted 6.3.0 as well. The narrower point I 
would make is that `ConnectionMetaData` is consumed programmatically at 
runtime, so a caveat documented on the website cannot reach a library that asks 
the provider directly. Happy to open a PR for whichever direction you prefer.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact


Reply via email to