Skip to content

Commit 7d8b067

Browse files
committed
Bug 1816258 - Websocket opening takes ages after repeated failures to same address r=necko-reviewers,valentin
Although our implementation follows the intention of rfc6455#section-4.1, we can handle the described situation more gracefully by prioritizing new WS connections to paths that have _not_ previously failed. Differential Revision: https://phabricator.services.mozilla.com/D193386
1 parent 2066c0b commit 7d8b067

File tree

2 files changed

+68
-26
lines changed

2 files changed

+68
-26
lines changed

netwerk/protocol/websocket/WebSocketChannel.cpp

Lines changed: 67 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -121,11 +121,11 @@ const uint32_t kWSReconnectBaseLifeTime = 60 * 1000;
121121
const uint32_t kWSReconnectMaxDelay = 60 * 1000;
122122

123123
// hold record of failed connections, and calculates needed delay for reconnects
124-
// to same host/port.
124+
// to same host/path/port.
125125
class FailDelay {
126126
public:
127-
FailDelay(nsCString address, int32_t port)
128-
: mAddress(std::move(address)), mPort(port) {
127+
FailDelay(nsCString address, nsCString path, int32_t port)
128+
: mAddress(std::move(address)), mPath(std::move(path)), mPort(port) {
129129
mLastFailure = TimeStamp::Now();
130130
mNextDelay = kWSReconnectInitialBaseDelay +
131131
(rand() % kWSReconnectInitialRandomDelay);
@@ -139,9 +139,10 @@ class FailDelay {
139139
mNextDelay = static_cast<uint32_t>(
140140
std::min<double>(kWSReconnectMaxDelay, mNextDelay * 1.5));
141141
LOG(
142-
("WebSocket: FailedAgain: host=%s, port=%d: incremented delay to "
142+
("WebSocket: FailedAgain: host=%s, path=%s, port=%d: incremented delay "
143+
"to "
143144
"%" PRIu32,
144-
mAddress.get(), mPort, mNextDelay));
145+
mAddress.get(), mPath.get(), mPort, mNextDelay));
145146
}
146147

147148
// returns 0 if there is no need to delay (i.e. delay interval is over)
@@ -160,6 +161,7 @@ class FailDelay {
160161
}
161162

162163
nsCString mAddress; // IP address (or hostname if using proxy)
164+
nsCString mPath;
163165
int32_t mPort;
164166

165167
private:
@@ -191,16 +193,16 @@ class FailDelayManager {
191193

192194
~FailDelayManager() { MOZ_COUNT_DTOR(FailDelayManager); }
193195

194-
void Add(nsCString& address, int32_t port) {
196+
void Add(nsCString& address, nsCString& path, int32_t port) {
195197
if (mDelaysDisabled) return;
196198

197-
UniquePtr<FailDelay> record(new FailDelay(address, port));
199+
UniquePtr<FailDelay> record(new FailDelay(address, path, port));
198200
mEntries.AppendElement(std::move(record));
199201
}
200202

201203
// Element returned may not be valid after next main thread event: don't keep
202204
// pointer to it around
203-
FailDelay* Lookup(nsCString& address, int32_t port,
205+
FailDelay* Lookup(nsCString& address, nsCString& path, int32_t port,
204206
uint32_t* outIndex = nullptr) {
205207
if (mDelaysDisabled) return nullptr;
206208

@@ -211,7 +213,8 @@ class FailDelayManager {
211213
// indexing simpler
212214
for (int32_t i = mEntries.Length() - 1; i >= 0; --i) {
213215
FailDelay* fail = mEntries[i].get();
214-
if (fail->mAddress.Equals(address) && fail->mPort == port) {
216+
if (fail->mAddress.Equals(address) && fail->mPath.Equals(path) &&
217+
fail->mPort == port) {
215218
if (outIndex) *outIndex = i;
216219
result = fail;
217220
// break here: removing more entries would mess up *outIndex.
@@ -230,7 +233,7 @@ class FailDelayManager {
230233
void DelayOrBegin(WebSocketChannel* ws) {
231234
if (!mDelaysDisabled) {
232235
uint32_t failIndex = 0;
233-
FailDelay* fail = Lookup(ws->mAddress, ws->mPort, &failIndex);
236+
FailDelay* fail = Lookup(ws->mAddress, ws->mPath, ws->mPort, &failIndex);
234237

235238
if (fail) {
236239
TimeStamp rightNow = TimeStamp::Now();
@@ -266,13 +269,14 @@ class FailDelayManager {
266269

267270
// Remove() also deletes all expired entries as it iterates: better for
268271
// battery life than using a periodic timer.
269-
void Remove(nsCString& address, int32_t port) {
272+
void Remove(nsCString& address, nsCString& path, int32_t port) {
270273
TimeStamp rightNow = TimeStamp::Now();
271274

272275
// iterate from end, to make deletion indexing easier
273276
for (int32_t i = mEntries.Length() - 1; i >= 0; --i) {
274277
FailDelay* entry = mEntries[i].get();
275-
if ((entry->mAddress.Equals(address) && entry->mPort == port) ||
278+
if ((entry->mAddress.Equals(address) && entry->mPath.Equals(path) &&
279+
entry->mPort == port) ||
276280
entry->IsExpired(rightNow)) {
277281
mEntries.RemoveElementAt(i);
278282
}
@@ -321,14 +325,30 @@ class nsWSAdmissionManager {
321325

322326
// If there is already another WS channel connecting to this IP address,
323327
// defer BeginOpen and mark as waiting in queue.
324-
bool found = (sManager->IndexOf(ws->mAddress, ws->mOriginSuffix) >= 0);
328+
bool hostFound = (sManager->IndexOf(ws->mAddress, ws->mOriginSuffix) >= 0);
329+
330+
uint32_t failIndex = 0;
331+
FailDelay* fail = sManager->mFailures.Lookup(ws->mAddress, ws->mPath,
332+
ws->mPort, &failIndex);
333+
bool existingFail = fail != nullptr;
325334

326335
// Always add ourselves to queue, even if we'll connect immediately
327336
UniquePtr<nsOpenConn> newdata(
328-
new nsOpenConn(ws->mAddress, ws->mOriginSuffix, ws));
329-
sManager->mQueue.AppendElement(std::move(newdata));
337+
new nsOpenConn(ws->mAddress, ws->mOriginSuffix, existingFail, ws));
330338

331-
if (found) {
339+
// If a connection has not previously failed then prioritize it over
340+
// connections that have
341+
if (existingFail) {
342+
sManager->mQueue.AppendElement(std::move(newdata));
343+
} else {
344+
uint32_t insertionIndex = sManager->IndexOfFirstFailure();
345+
MOZ_ASSERT(insertionIndex >= 0, "Insertion index positive");
346+
MOZ_ASSERT(insertionIndex <= sManager->mQueue.Length(),
347+
"Insertion index outside bounds");
348+
sManager->mQueue.InsertElementAt(insertionIndex, std::move(newdata));
349+
}
350+
351+
if (hostFound) {
332352
LOG(
333353
("Websocket: some other channel is connecting, changing state to "
334354
"CONNECTING_QUEUED"));
@@ -357,7 +377,8 @@ class nsWSAdmissionManager {
357377
sManager->RemoveFromQueue(aChannel);
358378

359379
// Connection succeeded, so stop keeping track of any previous failures
360-
sManager->mFailures.Remove(aChannel->mAddress, aChannel->mPort);
380+
sManager->mFailures.Remove(aChannel->mAddress, aChannel->mPath,
381+
aChannel->mPort);
361382

362383
// Check for queued connections to same host.
363384
// Note: still need to check for failures, since next websocket with same
@@ -378,24 +399,27 @@ class nsWSAdmissionManager {
378399

379400
if (NS_FAILED(aReason)) {
380401
// Have we seen this failure before?
381-
FailDelay* knownFailure =
382-
sManager->mFailures.Lookup(aChannel->mAddress, aChannel->mPort);
402+
FailDelay* knownFailure = sManager->mFailures.Lookup(
403+
aChannel->mAddress, aChannel->mPath, aChannel->mPort);
383404
if (knownFailure) {
384405
if (aReason == NS_ERROR_NOT_CONNECTED) {
385406
// Don't count close() before connection as a network error
386407
LOG(
387-
("Websocket close() before connection to %s, %d completed"
408+
("Websocket close() before connection to %s, %s, %d completed"
388409
" [this=%p]",
389-
aChannel->mAddress.get(), (int)aChannel->mPort, aChannel));
410+
aChannel->mAddress.get(), aChannel->mPath.get(),
411+
(int)aChannel->mPort, aChannel));
390412
} else {
391413
// repeated failure to connect: increase delay for next connection
392414
knownFailure->FailedAgain();
393415
}
394416
} else {
395417
// new connection failure: record it.
396-
LOG(("WebSocket: connection to %s, %d failed: [this=%p]",
397-
aChannel->mAddress.get(), (int)aChannel->mPort, aChannel));
398-
sManager->mFailures.Add(aChannel->mAddress, aChannel->mPort);
418+
LOG(("WebSocket: connection to %s, %s, %d failed: [this=%p]",
419+
aChannel->mAddress.get(), aChannel->mPath.get(),
420+
(int)aChannel->mPort, aChannel));
421+
sManager->mFailures.Add(aChannel->mAddress, aChannel->mPath,
422+
aChannel->mPort);
399423
}
400424
}
401425

@@ -480,15 +504,19 @@ class nsWSAdmissionManager {
480504

481505
class nsOpenConn {
482506
public:
483-
nsOpenConn(nsCString& addr, nsCString& originSuffix,
507+
nsOpenConn(nsCString& addr, nsCString& originSuffix, bool failed,
484508
WebSocketChannel* channel)
485-
: mAddress(addr), mOriginSuffix(originSuffix), mChannel(channel) {
509+
: mAddress(addr),
510+
mOriginSuffix(originSuffix),
511+
mFailed(failed),
512+
mChannel(channel) {
486513
MOZ_COUNT_CTOR(nsOpenConn);
487514
}
488515
MOZ_COUNTED_DTOR(nsOpenConn)
489516

490517
nsCString mAddress;
491518
nsCString mOriginSuffix;
519+
bool mFailed = false;
492520
RefPtr<WebSocketChannel> mChannel;
493521
};
494522

@@ -535,6 +563,15 @@ class nsWSAdmissionManager {
535563
return -1;
536564
}
537565

566+
// Returns the index of the first entry that failed, or else the last entry if
567+
// none found
568+
uint32_t IndexOfFirstFailure() {
569+
for (uint32_t i = 0; i < mQueue.Length(); i++) {
570+
if (mQueue[i]->mFailed) return i;
571+
}
572+
return mQueue.Length();
573+
}
574+
538575
// SessionCount might be decremented from the main or the socket
539576
// thread, so manage it with atomic counters
540577
Atomic<int32_t> mSessionCount;
@@ -2853,6 +2890,10 @@ nsresult WebSocketChannel::DoAdmissionDNS() {
28532890
rv = mURI->GetHost(hostName);
28542891
NS_ENSURE_SUCCESS(rv, rv);
28552892
mAddress = hostName;
2893+
nsCString path;
2894+
rv = mURI->GetFilePath(path);
2895+
NS_ENSURE_SUCCESS(rv, rv);
2896+
mPath = path;
28562897
rv = mURI->GetPort(&mPort);
28572898
NS_ENSURE_SUCCESS(rv, rv);
28582899
if (mPort == -1) mPort = (mEncrypted ? kDefaultWSSPort : kDefaultWSPort);

netwerk/protocol/websocket/WebSocketChannel.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,7 @@ class WebSocketChannel : public BaseWebSocketChannel,
229229
// then to IP address (unless we're leaving DNS resolution to a proxy server)
230230
// MainThread only
231231
nsCString mAddress;
232+
nsCString mPath;
232233
int32_t mPort; // WS server port
233234
// Secondary key for the connection queue. Used by nsWSAdmissionManager.
234235
nsCString mOriginSuffix; // MainThread only

0 commit comments

Comments
 (0)