Split apart capability queue into individual and bundled groups.
Request capabilities one-at-a-time in the individual queue, and as
many as will fit within length limits in the bundled queue. Use a
length limit of 100 characters to follow minimum number of characters
that IRC servers must return in CAP NAK replies, also meaning CAP NAK
replies will contain the full list of denied capabilities.
Individually request SASL and other capabilities requiring
configuration to avoid conflicts with requesting new capabilities
while still setting up the current capability.
Retry bundled capability requests individually when failed. This
prevents one failing capability from blocking others. Unfortunately
there's no way to avoid blindly re-requesting as CAP NAK does not
specify which capability failed.
Show a warning when retrying capabilities individually to explain the
added delay in logging in and to ease troubleshooting.
Fix documentation regarding capability handling.
Resolves GH-221.
_quitReason.clear();
// Reset capability negotiation tracking, also handling server changes during reconnect
- _capsQueued.clear();
+ _capsQueuedIndividual.clear();
+ _capsQueuedBundled.clear();
clearCaps();
_capNegotiationActive = false;
_capInitialNegotiationEnded = false;
{
// IRCv3 specs all use lowercase capability names
QString _capLowercase = capability.toLower();
- if (!_capsQueued.contains(_capLowercase)) {
- _capsQueued.append(_capLowercase);
+
+ if(capsRequiringConfiguration.contains(_capLowercase)) {
+ // The capability requires additional configuration before being acknowledged (e.g. SASL),
+ // so we should negotiate it separately from all other capabilities. Otherwise new
+ // capabilities will be requested while still configuring the previous one.
+ if (!_capsQueuedIndividual.contains(_capLowercase)) {
+ _capsQueuedIndividual.append(_capLowercase);
+ }
+ } else {
+ // The capability doesn't need any special configuration, so it should be safe to try
+ // bundling together with others. "Should" being the imperative word, as IRC servers can do
+ // anything.
+ if (!_capsQueuedBundled.contains(_capLowercase)) {
+ _capsQueuedBundled.append(_capLowercase);
+ }
}
}
-QString CoreNetwork::takeQueuedCap()
-{
- if (!_capsQueued.empty()) {
- return _capsQueued.takeFirst();
+QString CoreNetwork::takeQueuedCaps()
+{
+ // Clear the record of the most recently negotiated capability bundle. Does nothing if the list
+ // is empty.
+ _capsQueuedLastBundle.clear();
+
+ // First, negotiate all the standalone capabilities that require additional configuration.
+ if (!_capsQueuedIndividual.empty()) {
+ // We have an individual capability available. Take the first and pass it back.
+ return _capsQueuedIndividual.takeFirst();
+ } else if (!_capsQueuedBundled.empty()) {
+ // We have capabilities available that can be grouped. Try to fit in as many as within the
+ // maximum length.
+ // See CoreNetwork::maxCapRequestLength
+
+ // Response must have at least one capability regardless of max length for anything to
+ // happen.
+ QString capBundle = _capsQueuedBundled.takeFirst();
+ QString nextCap("");
+ while (!_capsQueuedBundled.empty()) {
+ // As long as capabilities remain, get the next...
+ nextCap = _capsQueuedBundled.first();
+ if ((capBundle.length() + 1 + nextCap.length()) <= maxCapRequestLength) {
+ // [capability + 1 for a space + this new capability] fit within length limits
+ // Add it to formatted list
+ capBundle.append(" " + nextCap);
+ // Add it to most recent bundle of requested capabilities (simplifies retry logic)
+ _capsQueuedLastBundle.append(nextCap);
+ // Then remove it from the queue
+ _capsQueuedBundled.removeFirst();
+ } else {
+ // We've reached the length limit for a single capability request, stop adding more
+ break;
+ }
+ }
+ // Return this space-separated set of capabilities, removing any extra spaces
+ return capBundle.trimmed();
} else {
+ // No capabilities left to negotiate, return an empty string.
return QString();
}
}
+void CoreNetwork::retryCapsIndividually()
+{
+ // The most recent set of capabilities got denied by the IRC server. As we don't know what got
+ // denied, try each capability individually.
+ if (_capsQueuedLastBundle.empty()) {
+ // No most recently tried capability set, just return.
+ return;
+ // Note: there's little point in retrying individually requested caps during negotiation.
+ // We know the individual capability was the one that failed, and it's not likely it'll
+ // suddenly start working within a few seconds. 'cap-notify' provides a better system for
+ // handling capability removal and addition.
+ }
+
+ // This should be fairly rare, e.g. services restarting during negotiation, so simplicity wins
+ // over efficiency. If this becomes an issue, implement a binary splicing system instead,
+ // keeping track of which halves of the group fail, dividing the set each time.
+
+ // Add most recently tried capability set to individual list, re-requesting them one at a time
+ _capsQueuedIndividual.append(_capsQueuedLastBundle);
+ // Warn of this issue to explain the slower login. Servers usually shouldn't trigger this.
+ displayMsg(Message::Server, BufferInfo::StatusBuffer, "",
+ tr("Could not negotiate some capabilities, retrying individually (%1)...")
+ .arg(_capsQueuedLastBundle.join(", ")));
+ // Capabilities are already removed from the capability bundle queue via takeQueuedCaps(), no
+ // need to remove them here.
+ // Clear the most recently tried set to reduce risk that mistakes elsewhere causes retrying
+ // indefinitely.
+ _capsQueuedLastBundle.clear();
+}
+
void CoreNetwork::beginCapNegotiation()
{
// Don't begin negotiation if no capabilities are queued to request
_capNegotiationActive = true;
displayMsg(Message::Server, BufferInfo::StatusBuffer, "",
tr("Ready to negotiate (found: %1)").arg(caps().join(", ")));
+
+ // Build a list of queued capabilities, starting with individual, then bundled, only adding the
+ // comma separator between the two if needed.
+ QString queuedCapsDisplay =
+ (!_capsQueuedIndividual.empty() ? _capsQueuedIndividual.join(", ") + ", " : "")
+ + _capsQueuedBundled.join(", ");
displayMsg(Message::Server, BufferInfo::StatusBuffer, "",
- tr("Negotiating capabilities (requesting: %1)...").arg(_capsQueued.join(", ")));
+ tr("Negotiating capabilities (requesting: %1)...").arg(queuedCapsDisplay));
+
sendNextCap();
}
void CoreNetwork::sendNextCap()
{
if (capNegotiationInProgress()) {
- // Request the next capability and remove it from the list
- // Handle one at a time so one capability failing won't NAK all of 'em
- putRawLine(serverEncode(QString("CAP REQ :%1").arg(takeQueuedCap())));
+ // Request the next set of capabilities and remove them from the list
+ putRawLine(serverEncode(QString("CAP REQ :%1").arg(takeQueuedCaps())));
} else {
// No pending desired capabilities, capability negotiation finished
// If SASL requested but not available, print a warning
*
* @returns True if in progress, otherwise false
*/
- inline bool capNegotiationInProgress() const { return !_capsQueued.empty(); }
+ inline bool capNegotiationInProgress() const { return (!_capsQueuedIndividual.empty() ||
+ !_capsQueuedBundled.empty()); }
/**
* Queues a capability to be requested.
*/
void endCapNegotiation();
+ /**
+ * Queues the most recent capability set for retrying individually.
+ *
+ * Retries the most recent bundle of capabilities one at a time instead of as a group, working
+ * around the issue that IRC servers can deny a group of requested capabilities without
+ * indicating which capabilities failed.
+ *
+ * See: http://ircv3.net/specs/core/capability-negotiation-3.1.html
+ *
+ * This does NOT call CoreNetwork::sendNextCap(). Call that when ready afterwards. Does
+ * nothing if the last capability tried was individual instead of a set.
+ */
+ void retryCapsIndividually();
+
/**
* List of capabilities requiring further core<->server messages to configure.
*
* For example, SASL requires the back-and-forth of AUTHENTICATE, so the next capability cannot
* be immediately sent.
*
+ * Any capabilities in this list must call CoreNetwork::sendNextCap() on their own and they will
+ * not be batched together with other capabilities.
+ *
* See: http://ircv3.net/specs/extensions/sasl-3.2.html
*/
const QStringList capsRequiringConfiguration = QStringList {
// Maintain a list of CAPs that are being checked; if empty, negotiation finished
// See http://ircv3.net/specs/core/capability-negotiation-3.2.html
- QStringList _capsQueued; /// Capabilities to be checked
- bool _capNegotiationActive; /// Whether or not full capability negotiation was started
+ QStringList _capsQueuedIndividual; /// Capabilities to check that require one at a time requests
+ QStringList _capsQueuedBundled; /// Capabilities to check that can be grouped together
+ QStringList _capsQueuedLastBundle; /// Most recent capability bundle requested (no individuals)
+ // Some capabilities, such as SASL, require follow-up messages to be fully enabled. These
+ // capabilities should not be grouped with others to avoid requesting new capabilities while the
+ // previous capability is still being set up.
+ // Additionally, IRC servers can choose to send a 'NAK' to any set of requested capabilities.
+ // If this happens, we need a way to retry each capability individually in order to avoid having
+ // one failing capability (e.g. SASL) block all other capabilities.
+
+ bool _capNegotiationActive; /// Whether or not full capability negotiation was started
// Avoid displaying repeat "negotiation finished" messages
- bool _capInitialNegotiationEnded; /// Whether or not initial capability negotiation finished
+ bool _capInitialNegotiationEnded; /// Whether or not initial capability negotiation finished
// Avoid sending repeat "CAP END" replies when registration is already ended
/**
- * Gets the next capability to request, removing it from the queue.
+ * Gets the next set of capabilities to request, removing them from the queue.
+ *
+ * May return one or multiple space-separated capabilities, depending on queue.
+ *
+ * @returns Space-separated names of capabilities to request, or empty string if none remain
+ */
+ QString takeQueuedCaps();
+
+ /**
+ * Maximum length of a single 'CAP REQ' command.
+ *
+ * To be safe, 100 chars. Higher numbers should be possible; this is following the conservative
+ * minimum number of characters that IRC servers must return in CAP NAK replies. This also
+ * means CAP NAK replies will contain the full list of denied capabilities.
*
- * @returns Name of capability to request
+ * See: http://ircv3.net/specs/core/capability-negotiation-3.1.html
*/
- QString takeQueuedCap();
+ const int maxCapRequestLength = 100;
QTimer _tokenBucketTimer;
int _messageDelay; // token refill speed in ms
}
// Server: CAP * ACK :multi-prefix sasl
- // Got the capability we want, handle as needed.
- // As only one capability is requested at a time, no need to split
- QString acceptedCap = e->params().at(2).trimmed().toLower();
-
- // Mark this cap as accepted
- coreNet->acknowledgeCap(acceptedCap);
+ // Got the capabilities we want, handle as needed.
+ QStringList acceptedCaps;
+ acceptedCaps = e->params().at(2).split(' ');
+
+ // Store what capability was acknowledged
+ QString acceptedCap;
+
+ // Keep track of whether or not a capability requires further configuration. Due to queuing
+ // logic in CoreNetwork::queueCap(), this shouldn't ever happen when more than one
+ // capability is requested, but it's better to handle edge cases or faulty servers.
+ bool capsRequireConfiguration = false;
+
+ for (int i = 0; i < acceptedCaps.count(); ++i) {
+ acceptedCap = acceptedCaps[i].trimmed().toLower();
+ // Mark this cap as accepted
+ coreNet->acknowledgeCap(acceptedCap);
+ if (!capsRequireConfiguration &&
+ coreNet->capsRequiringConfiguration.contains(acceptedCap)) {
+ capsRequireConfiguration = true;
+ // Some capabilities (e.g. SASL) require further messages to finish. If so, do NOT
+ // send the next capability; it will be handled elsewhere in CoreNetwork.
+ // Otherwise, allow moving on to the next capability.
+ }
+ }
- if (!coreNet->capsRequiringConfiguration.contains(acceptedCap)) {
- // Some capabilities (e.g. SASL) require further messages to finish. If so, do NOT
- // send the next capability; it will be handled elsewhere in CoreNetwork.
- // Otherwise, move on to the next capability
+ if (!capsRequireConfiguration) {
+ // No additional configuration required, move on to the next capability
coreNet->sendNextCap();
}
} else if (capCommand == "NAK" || capCommand == "DEL") {
return;
}
- // Either something went wrong with this capability, or it is no longer supported
+ // Either something went wrong with the capabilities, or they are no longer supported
// > For CAP NAK
// Server: CAP * NAK :multi-prefix sasl
// > For CAP DEL
QStringList removedCaps;
removedCaps = e->params().at(2).split(' ');
- // Store what capability was denied or removed
+ // Store the capabilities that were denied or removed
QString removedCap;
for (int i = 0; i < removedCaps.count(); ++i) {
removedCap = removedCaps[i].trimmed().toLower();
- // Mark this cap as removed
+ // Mark this cap as removed.
+ // For CAP DEL, removes it from use.
+ // For CAP NAK when received before negotiation enabled these capabilities, removeCap()
+ // should do nothing. This merely guards against non-spec servers sending an
+ // unsolicited CAP ACK then later removing that capability.
coreNet->removeCap(removedCap);
}
if (capCommand == "NAK") {
- // Continue negotiation only if this is the result of a denied cap, not a removed
- // cap
+ // Continue negotiation only if this is the result of denied caps, not removed caps
+ if (removedCaps.count() > 1) {
+ // We've received a CAP NAK reply to multiple capabilities at once. Unfortunately,
+ // we don't know which capability failed and which ones are valid to re-request, so
+ // individually retry each capability from the failed bundle.
+ // See CoreNetwork::retryCapsIndividually() for more details.
+ coreNet->retryCapsIndividually();
+ // Still need to call sendNextCap() to carry on.
+ }
+ // Carry on with negotiation
coreNet->sendNextCap();
}
}
projects / quassel.git / commitdiff