You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "ldauvilaire (via GitHub)" <gi...@apache.org> on 2024/09/04 13:17:03 UTC
[PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
ldauvilaire opened a new pull request, #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207
Avoid a NullPointerException that can occur on openwire reconnections.
Seen stack trace :
> Errors occurred during the buffering operation : java.lang.NullPointerException
at org.apache.activemq.artemis.core.protocol.openwire.OpenWireConnection$CommandProcessor.processAddSession(OpenWireConnection.java:1491)
at org.apache.activemq.command.SessionInfo.visit(SessionInfo.java:66)
at org.apache.activemq.artemis.core.protocol.openwire.OpenWireConnection.act(OpenWireConnection.java:366)
at org.apache.activemq.artemis.core.protocol.openwire.OpenWireConnection.bufferReceived(OpenWireConnection.java:309)
at org.apache.activemq.artemis.core.remoting.server.impl.RemotingServiceImpl$DelegatingBufferHandler.bufferReceived(RemotingServiceImpl.java:723)
at org.apache.activemq.artemis.core.remoting.impl.netty.ActiveMQChannelHandler.channelRead(ActiveMQChannelHandler.java:73)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:442)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412)
at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:346)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:318)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412)
at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1475)
at io.netty.handler.ssl.SslHandler.decodeJdkCompatible(SslHandler.java:1338)
at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1387)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:529)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:468)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:290)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412)
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:440)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2329951176
Something else I noticed...
When the OpenWire JMS client invokes `ConnectionFactory.createConnection()` the broker receives an OpenWire `ConnectionInfo` command where `responseRequired` is `true` which, as far as I can tell, means the client should wait to receive a response from the broker before continuing which therefore means `state` should not be `null` when the broker runs `processAddSession`. Could you perhaps reproduce this when running the broker on a JDK >= 15 so the NPE would include specific details about what is `null`?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2329974137
I have only tested with Java 11.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2332307760
The `internalSession` is initialized when the broker handles the OpenWire `ConnectionInfo` command in `org.apache.activemq.artemis.core.protocol.openwire.OpenWireConnection.CommandProcessor#processAddConnection`. Given that is apparently not finished when other OpenWire commands come in on the same connection the NPE fixed in `processAddSession` just delays another NPE later due to the same problem.
We really need to get to the bottom of the original problem which is...Why is the OpenWire client proceeding with additional commands when the `ConnectionInfo` command hasn't been handled yet?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2333971090
The method "processAddConnection()" is handling the processing of the ConnectionInfo Command.
In case of exception into the processing of "protocolManager.validateUser()" (due to SecurityException for example), a ExceptionResponse is provided but "protocolManager.addConnection()" is not called and then context is not initialized and then the state is not computed.
The ExceptionResponse generated in case of a ConnectionInfo Command then generates a call to "delayedStop(2000, cause.getMessage(), cause)".
During that time (2000ms), the connection can still receive and handle other commands, but as the state is not build, the commands may also fail.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2332112815
I have re-tested. the issue comes with state is null.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "mattrpav (via GitHub)" <gi...@apache.org>.
mattrpav commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2341606099
I'm not sure this change is appropriate for all use cases. I've created a JIRA in the ActiveMQ project to review. https://issues.apache.org/jira/browse/AMQ-9564
```
@Override
public void onException(JMSException exception) {
if (isReconnectOnException() || (exception instanceof JMSSecurityException)) {
close();
}
if (parentExceptionListener != null) {
parentExceptionListener.onException(exception);
}
}
```
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2331673803
no possible for me to test with a version of Java > 11 for the moment.
(activemq-artemis running as embedded into a jboss eap 7.4 with a custom Login-Module not running after Java 11 due to jboss no more supporting "deprecated" security system)
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2332128121
@ldauvilaire, please squash the commits so there's just one. There's no need to have one commit adding something and then another commit removing it. Thanks!
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2341402086
with the patch for the state into this PR + change into the PooledConnectionFactory to reconnectOnException, then it works fine.
when authenticated user gets null, then a SecurityException is transmitted to the client, then the client reconnects with new username/password then it works fine.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2341814388
> I was actually about to merge this PR to resolve NPE, but then you dropped a new commit on it related to improving the `ExceptionResponse`. I think that change, if desired, should be on a separate PR and this PR should only be about the NPE. Avoiding the NPE and using `true` for `reconnectOnException` on the pool resolves the issue(s), no?
ok. I can revert then.
do I keep the unit test ?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2341809401
I was actually about to merge this PR to resolve NPE, but then you dropped a new commit on it related to improving the `ExceptionResponse`. I think that change, if desired, should be on a separate PR and this PR should only be about the NPE. Avoiding the NPE and using `true` for `reconnectOnException` on the pool resolves the issue(s), no?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2341830354
There's no need to keep the test. Mitigating the NPE is straight-forward and clear from static analysis. Feel free to open another PR with the test and the improvement to the `ExceptionResponse`, but please clarify exactly why the improvement is helpful/necessary.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2332182261
Anyway with the fix, there are still some NullPointerExceptions like this one :
> Errors occurred during the buffering operation : java.lang.NullPointerException
at org.apache.activemq.artemis.core.protocol.openwire.OpenWireConnection$CommandProcessor.processBeginTransaction(OpenWireConnection.java:1
546)
at org.apache.activemq.command.TransactionInfo.visit(TransactionInfo.java:94)
at org.apache.activemq.artemis.core.protocol.openwire.OpenWireConnection.act(OpenWireConnection.java:366)
at org.apache.activemq.artemis.core.protocol.openwire.OpenWireConnection.bufferReceived(OpenWireConnection.java:309)
at org.apache.activemq.artemis.core.remoting.server.impl.RemotingServiceImpl$DelegatingBufferHandler.bufferReceived(RemotingServiceImpl.jav
a:723)
at org.apache.activemq.artemis.core.remoting.impl.netty.ActiveMQChannelHandler.channelRead(ActiveMQChannelHandler.java:73)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:442)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412)
at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:346)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:318)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412)
at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1475)
at io.netty.handler.ssl.SslHandler.decodeJdkCompatible(SslHandler.java:1338)
at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1387)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:529)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:468)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:290)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412)
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:440)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
at io.netty.channel.epoll.AbstractEpollStreamChannel$EpollStreamUnsafe.epollInReady(AbstractEpollStreamChannel.java:800)
at io.netty.channel.epoll.EpollEventLoop.processReady(EpollEventLoop.java:509)
at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:407)
at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997)
at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
at org.apache.activemq.artemis.utils.ActiveMQThreadFactory$1.run(ActiveMQThreadFactory.java:118)
Seems more annoing because the code corresponds to
` } finally {
internalSession.resetTX(null);
}
`
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "mattrpav (via GitHub)" <gi...@apache.org>.
mattrpav commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2341420551
@ldauvilaire where is the change for activemq-pool? We have a 5.18.x release in-progress and can look to incorporate it in the next set of release.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram merged PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "mattrpav (via GitHub)" <gi...@apache.org>.
mattrpav commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2341649371
@ldauvilaire reconnecting on every security exception may not be valid.
For example:
1. App uses connectionA requests producer to queue://order (success)
2. App returns connection to pool
3. App requests connection (gets connectionA) and requests a producer to queue://quote (security failure b/w there is no permission)
Closing and re-opening is not going to fix the issue. That exception _should_ be bubbled up to the client app code.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2329906926
> ...when the NPE occurs the consequence is some other exception with message « Cannot add a producer to a connection that had not been registered » ; the error is not transmitted correctly to the client and the client retries to use the same connection again and again.
In what sense is the error "not transmitted correctly to the client"? When I modify the broker to reproduce an equivalent `NullPointerException` when calling `createSession` the subsequent call to `createProducer` fails with a `javax.jms.IllegalStateException` which seems appropriate. Might the `PooledConnectionFactory` or perhaps the application using the pool have a bug where it's not interpreting this exception properly? Such an exception when invoking `createProducer` indicates an "internal error" so the safest thing would be to destroy the corresponding `Session` and `Connection` and re-create them.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2329719316
Do you have a way to reproduce this?
I'm concerned that simply logging a `WARN` message may not be sufficient here. We may need to enforce some concurrency controls as it looks like the OpenWire client is attempting to create a session before the broker has finished processing the command to create the connection.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2341649504
Regarding this PR, I can add a small improvement on the ExceptionResponse and add a unit test
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2337650790
It seems that the configuration of the PooledConnectionFactory has an impact.
When "reconnectOnException" is set to true, then the issue is not present.
I am also checking that parameter "useAnonymousProducers" ...
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2331912811
The class `org.apache.activemq.artemis.core.protocol.openwire.OpenWireConnection` in ActiveMQ Artemis is based on `org.apache.activemq.broker.TransportConnection` from ActiveMQ Classic, and in the latter there is `null` check that doesn't exist in the former. Therefore, I think it's worth adding this check, e.g.:
```java
@Override
public Response processAddSession(SessionInfo info) throws Exception {
// Avoid replaying dup commands
if (state != null && !state.getSessionIds().contains(info.getSessionId())) {
addSession(info);
state.addSession(info);
}
return null;
}
```
The `info` should never be `null` since it is passed in off the wire from the OpenWire marshaller, and `state.getSessionIds()` should also never return `null` because the underlying `ConcurrentMap` is initialized when `ConnectionState` is created.
Past that I think it's up to the client (i.e. the application or the PooledConnectionFactory) to deal with the `javax.jms.IllegalStateException` from `createProducer` more thoughtfully.
Please amend your commit with this change and also reference ARTEMIS-5033 (which I just opened for this) in your commit message. Additional details on commit message format can be found [here](https://activemq.apache.org/components/artemis/documentation/hacking-guide/#commitMessageDetails).
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2330253877
> I have only tested with Java 11.
Could you test with Java 15 or higher on the broker?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2329752812
It can be reproducted with a client using a PooledConnectionFactory with authenticated connections (with username and password) when the activemq artemis server is restarted. The use case is not systematic but it happens.
Yes this PR is more a workaround than a correct fix, but when the NPE occurs the consequence is some other exception with message « Cannot add a producer to a connection that had not been registered » ; the error is not transmitted correctly to the client and the client retries to use the same connection again and again.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2334049163
> In case of exception into the processing of "protocolManager.validateUser()" (due to SecurityException for example), a ExceptionResponse is provided but "protocolManager.addConnection()" is not called and then context is not initialized and then the state is not computed.
The ExceptionResponse generated in case of a ConnectionInfo Command then generates a call to "delayedStop(2000, cause.getMessage(), cause)".
This is all expected behavior. The `delayedStop` is there to ensure that after the `ExceptionResponse` is returned to the client that the server-side resources are cleaned up (e.g. a socket isn't left half-closed or something).
> During that time (2000ms), the connection can still receive and handle other commands, but as the state is not build, the commands may also fail.
This is the mysterious part. If the client receives an `ExceptionResponse` when it sends a `ConnectionInfo` command then why does it continue working with that connection?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2329963056
I don’t know exactly.
I have the same, a first NullPointerException followed by a IllegalStateException.
Seems safe to destroy the sessions and the connection and re-create it in from the connection factory in such case.
My use-case is a username & password that has a limited time to live, if reused during too long by the pool it becomes invalid and the custom ActiveMQSecurityManager5 authenticate method will return null. The client then needs to detect it and re-create a new connection
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2329709569
Have you actually seen any of these corresponding NullPointerExceptions or is this change based on static analysis?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2341498539
@ldauvilaire, does it work _without_ the change to `org.apache.activemq.artemis.core.protocol.openwire.OpenWireConnection` in this PR?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2341835523
> Just rebase and drop all the commits but the one for the NPE. Then `push -f`.
done
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2334437342
Using JmsTemplate from Spring-Framework with a PooledConnectionFactory (from activemq-pool)
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2332126525
Commit pushed.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2334392769
could it be due to some version of the activemq-client on the client side ?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2334403848
> could it be due to some version of the activemq-client on the client side ?
What pooling mechanism are you using?
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2341582864
@jbertram : yes it works, there are NullPointerException followed by IllegalStateException on the server side, but it works.
client side logs :
> o.a.a.t.f.FailoverTransport Successfully connected to ssl://localhost:62617?verifyHostName=false&soTimeout=30000&soWriteTimeout=30000&keepAlive=true
>
> o.a.a.t.f.FailoverTransport Transport (ssl://localhost:62617?verifyHostName=false&soTimeout=30000&soWriteTimeout=30000&keepAlive=true) failed, attempting to automatically reconnect: java.io.EOFException
at java.io.DataInputStream.readInt(DataInputStream.java:392)
at org.apache.activemq.openwire.OpenWireFormat.unmarshal(OpenWireFormat.java:280)
at org.apache.activemq.transport.tcp.TcpTransport.readCommand(TcpTransport.java:240)
at org.apache.activemq.transport.tcp.TcpTransport.doRun(TcpTransport.java:232)
at org.apache.activemq.transport.tcp.TcpTransport.run(TcpTransport.java:215)
at java.lang.Thread.run(Thread.java:750)
>
> o.a.a.t.f.FailoverTransport Failed to connect to [ssl://localhost:62617?verifyHostName=false&soTimeout=30000&soWriteTimeout=30000&keepAlive=true] after: 1 attempt(s) with Connection refused (Connection refused), continuing to retry.
>
> o.a.a.t.f.FailoverTransport Failed to connect to [ssl://localhost:62617?verifyHostName=false&soTimeout=30000&soWriteTimeout=30000&keepAlive=true] after: 10 attempt(s) with Connection refused (Connection refused), continuing to retry.
>
> o.a.a.t.f.FailoverTransport Successfully reconnected to ssl://localhost:62617?verifyHostName=false&soTimeout=30000&soWriteTimeout=30000&keepAlive=true
@mattrpav : the change is in the configuration of the PooledConnectionFactory on the client side.
that's manageable on the client side.
I can suggest an improvement on the **artemis-jms-pool** in class **ConnectionPool** on method **onException**
to always close the connection in case of **JMSSecurityException**.
replacing
```
@Override
public void onException(JMSException exception) {
if (isReconnectOnException()) {
close();
}
if (parentExceptionListener != null) {
parentExceptionListener.onException(exception);
}
}
```
by
```
@Override
public void onException(JMSException exception) {
if (isReconnectOnException() || (exception instanceof JMSSecurityException)) {
close();
}
if (parentExceptionListener != null) {
parentExceptionListener.onException(exception);
}
}
```
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2341635081
@mattrpav , that's what I did on the client side.
by default reconnectOnException is false and it is not straightforward to find that reconnection issues can be linked to that parameter.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2341831747
Just rebase and drop all the commits but the one for the NPE.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Avoid NPE on method processAddSession in OpenWireConnection [activemq-artemis]
Posted by "ldauvilaire (via GitHub)" <gi...@apache.org>.
ldauvilaire commented on PR #5207:
URL: https://github.com/apache/activemq-artemis/pull/5207#issuecomment-2341697834
> @ldauvilaire reconnecting on every security exception may not be valid.
>
> For example:
>
> 1. App uses connectionA requests producer to queue://order (success)
> 2. App returns connection to pool
> 3. App requests connection (gets connectionA) and requests a producer to queue://quote (security failure b/w there is no permission)
>
> Closing and re-opening is not going to fix the issue. That exception _should_ be bubbled up to the client app code.
good point
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact