From f91044657fd4e8c1573513f0849faf83249ac7bd Mon Sep 17 00:00:00 2001 From: adiaholic Date: Sun, 19 May 2019 13:31:17 +0530 Subject: [PATCH 1/2] Generic Exception replaced with Type Specific Exceptions. 'parseAndProcessStanza()' throws generic Exceptions. Since there are plenty of exceptions that should not be catched by smack, it's better to throw Type Specific Exceptions. This Commit is was in response to SMACK-839. --- .../smack/AbstractXMPPConnection.java | 2 +- .../smack/util/PacketParserUtils.java | 16 +++++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java b/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java index 221a986b4..41890d14f 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java @@ -1251,7 +1251,7 @@ public abstract class AbstractXMPPConnection implements XMPPConnection { try { stanza = PacketParserUtils.parseStanza(parser, incomingStreamXmlEnvironment); } - catch (Exception e) { + catch (XmlPullParserException | SmackParsingException | IOException e) { CharSequence content = PacketParserUtils.parseContentDepth(parser, parserDepth); UnparseableStanza message = new UnparseableStanza(content, e); diff --git a/smack-core/src/main/java/org/jivesoftware/smack/util/PacketParserUtils.java b/smack-core/src/main/java/org/jivesoftware/smack/util/PacketParserUtils.java index e80c6efcc..520e4ee87 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/util/PacketParserUtils.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/util/PacketParserUtils.java @@ -55,6 +55,7 @@ import org.jivesoftware.smack.xml.XmlPullParser; import org.jivesoftware.smack.xml.XmlPullParserException; import org.jxmpp.jid.Jid; +import org.jxmpp.stringprep.XmppStringprepException; /** * Utility class that helps to parse packets. Any parsing packets method that must be shared @@ -89,7 +90,7 @@ public class PacketParserUtils { } @SuppressWarnings("unchecked") - public static S parseStanza(String stanza) throws Exception { + public static S parseStanza(String stanza) throws XmlPullParserException, SmackParsingException, IOException { return (S) parseStanza(getParserFor(stanza), null); } @@ -101,9 +102,11 @@ public class PacketParserUtils { * @param parser * @param outerXmlEnvironment the outer XML environment (optional). * @return a stanza which is either a Message, IQ or Presence. - * @throws Exception + * @throws XmlPullParserException + * @throws SmackParsingException + * @throws IOException */ - public static Stanza parseStanza(XmlPullParser parser, XmlEnvironment outerXmlEnvironment) throws Exception { + public static Stanza parseStanza(XmlPullParser parser, XmlEnvironment outerXmlEnvironment) throws XmlPullParserException, SmackParsingException, IOException { ParserUtils.assertAtStartTag(parser); final String name = parser.getName(); switch (name) { @@ -508,9 +511,12 @@ public class PacketParserUtils { * @param parser the XML parser, positioned at the start of an IQ packet. * @param outerXmlEnvironment the outer XML environment (optional). * @return an IQ object. - * @throws Exception + * @throws XmlPullParserException + * @throws XmppStringprepException + * @throws IOException + * @throws SmackParsingException */ - public static IQ parseIQ(XmlPullParser parser, XmlEnvironment outerXmlEnvironment) throws Exception { + public static IQ parseIQ(XmlPullParser parser, XmlEnvironment outerXmlEnvironment) throws XmlPullParserException, XmppStringprepException, IOException, SmackParsingException { ParserUtils.assertAtStartTag(parser); final int initialDepth = parser.getDepth(); XmlEnvironment iqXmlEnvironment = XmlEnvironment.from(parser, outerXmlEnvironment); From 02f7cfcf277b8c5f359134040674ece472dae308 Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Mon, 20 May 2019 08:57:03 +0200 Subject: [PATCH 2/2] Improve session sweeping logic in AdHocCommandManager Fixes SMACK-624. --- .../smackx/commands/AdHocCommandManager.java | 100 ++++++++---------- 1 file changed, 46 insertions(+), 54 deletions(-) diff --git a/smack-extensions/src/main/java/org/jivesoftware/smackx/commands/AdHocCommandManager.java b/smack-extensions/src/main/java/org/jivesoftware/smackx/commands/AdHocCommandManager.java index 8e60d6594..dd05857ae 100755 --- a/smack-extensions/src/main/java/org/jivesoftware/smackx/commands/AdHocCommandManager.java +++ b/smack-extensions/src/main/java/org/jivesoftware/smackx/commands/AdHocCommandManager.java @@ -20,10 +20,13 @@ package org.jivesoftware.smackx.commands; import java.lang.reflect.InvocationTargetException; import java.util.ArrayList; import java.util.Collection; +import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Map.Entry; import java.util.WeakHashMap; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.TimeUnit; import java.util.logging.Level; import java.util.logging.Logger; @@ -126,15 +129,6 @@ public final class AdHocCommandManager extends Manager { private final ServiceDiscoveryManager serviceDiscoveryManager; - /** - * Thread that reaps stale sessions. - */ - // FIXME The session sweeping is horrible implemented. The thread will never stop running. A different approach must - // be implemented. For example one that does stop reaping sessions and the thread if there are no more, and restarts - // the reaping process on demand. Or for every command a scheduled task should be created that removes the session - // if it's timed out. See SMACK-624. - private Thread sessionsSweeper; - private AdHocCommandManager(XMPPConnection connection) { super(connection); this.serviceDiscoveryManager = ServiceDiscoveryManager.getInstanceFor(connection); @@ -186,8 +180,6 @@ public final class AdHocCommandManager extends Manager { } } }); - - sessionsSweeper = null; } /** @@ -380,49 +372,8 @@ public final class AdHocCommandManager extends Manager { // available for the next call response.setStatus(Status.executing); executingCommands.put(sessionId, command); - // See if the session reaping thread is started. If not, start it. - if (sessionsSweeper == null) { - sessionsSweeper = new Thread(new Runnable() { - @Override - public void run() { - while (true) { - for (String sessionId : executingCommands.keySet()) { - LocalCommand command = executingCommands.get(sessionId); - // Since the command could be removed in the meanwhile - // of getting the key and getting the value - by a - // processed packet. We must check if it still in the - // map. - if (command != null) { - long creationStamp = command.getCreationDate(); - // Check if the Session data has expired (default is - // 10 minutes) - // To remove it from the session list it waits for - // the double of the of time out time. This is to - // let - // the requester know why his execution request is - // not accepted. If the session is removed just - // after the time out, then whe the user request to - // continue the execution he will received an - // invalid session error and not a time out error. - if (System.currentTimeMillis() - creationStamp > SESSION_TIMEOUT * 1000 * 2) { - // Remove the expired session - executingCommands.remove(sessionId); - } - } - } - try { - Thread.sleep(1000); - } - catch (InterruptedException ie) { - // Ignore. - } - } - } - - }); - sessionsSweeper.setDaemon(true); - sessionsSweeper.start(); - } + // See if the session sweeper thread is scheduled. If not, start it. + maybeWindUpSessionSweeper(); } // Sends the response packet @@ -558,6 +509,47 @@ public final class AdHocCommandManager extends Manager { } } + private boolean sessionSweeperScheduled; + + private final Runnable sessionSweeper = () -> { + final long currentTime = System.currentTimeMillis(); + synchronized (this) { + for (Iterator> it = executingCommands.entrySet().iterator(); it.hasNext();) { + Entry entry = it.next(); + LocalCommand command = entry.getValue(); + + long creationStamp = command.getCreationDate(); + // Check if the Session data has expired (default is 10 minutes) + // To remove it from the session list it waits for the double of + // the of time out time. This is to let + // the requester know why his execution request is + // not accepted. If the session is removed just + // after the time out, then once the user requests to + // continue the execution he will received an + // invalid session error and not a time out error. + if (currentTime - creationStamp > SESSION_TIMEOUT * 1000 * 2) { + // Remove the expired session + it.remove(); + } + } + + sessionSweeperScheduled = false; + } + + if (!executingCommands.isEmpty()) { + maybeWindUpSessionSweeper(); + } + }; + + private synchronized void maybeWindUpSessionSweeper() { + if (sessionSweeperScheduled) { + return; + } + + sessionSweeperScheduled = true; + schedule(sessionSweeper, 10, TimeUnit.SECONDS); + } + /** * Responds an error with an specific condition. *