New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clear silent connection attempt counter on DomainHandler soft reset #12195

Merged
merged 4 commits into from Feb 5, 2018

Conversation

Projects
None yet
5 participants
@birarda
Member

birarda commented Jan 17, 2018

  • fixed a bug where the domain connection would be reset early while negotiating connection for a logged in user

@birarda birarda added this to the RC64 milestone Jan 17, 2018

@highfidelity highfidelity deleted a comment from hifi-gustavo Jan 18, 2018

@highfidelity highfidelity deleted a comment from hifi-gustavo Jan 18, 2018

@highfidelity highfidelity deleted a comment from hifi-gustavo Jan 18, 2018

@highfidelity highfidelity deleted a comment from hifi-gustavo Jan 18, 2018

@highfidelity highfidelity deleted a comment from hifi-gustavo Jan 18, 2018

@birarda

This comment has been minimized.

Member

birarda commented Jan 18, 2018

build this please

@hifi-gustavo

This comment has been minimized.

@birarda birarda added QA Ready and removed QA Ready labels Jan 19, 2018

@hifi-gustavo

This comment has been minimized.

@birarda

This comment has been minimized.

Member

birarda commented Feb 2, 2018

Test Plan

Note that when looking at the log for this test plan you will need to enable the debug statements and may want to filter on hifi.networking.

  1. Using this PR, attempt to connect to "arcade", an offline domain. In the log, you should see "Limit of slient domain checkins reached" after 5 lines that say "Sending connect request to domain-server at". This cycle will repeat as your Interface continues to attempt to connect.
  2. Using this PR, attempt to connect a domain you do not have permissions to connect to. This can either be because the domain requires login and you are not logged in (for which you can test with recording.highfidelity.io) or because you are logged in but specifically denied connect permissions. In the log, you should see that that the domain-server is denying your connection request. You should NOT see "Limit of silent domain checkins reached" after 5 lines of "Sending connect request".
  3. Repeat step 2 using master build. You should see "Limit of silent domain checkins reached" after each group of 5 lines of "Sending connect request".
  4. Using master, switch between connecting to "arcade" and "wikiplanet" while logged out. You should see that "Limit of silent domain checkins reached" is shown after any group of 5 "Sending connect request" statements, even if you have changed domains in-between those statements. Using this PR, "Limit of silent domain checkins reached" should only appear if a group of 5 "Sending connect request" statements appears for the same domain. Basically, the count of unanswered "Sending connect request" statements should reset while changing domains with this PR, but does not with master.

@highfidelity highfidelity deleted a comment from hifi-gustavo Feb 2, 2018

@highfidelity highfidelity deleted a comment from hifi-gustavo Feb 2, 2018

@highfidelity highfidelity deleted a comment from hifi-gustavo Feb 2, 2018

@birarda birarda added the QA Ready label Feb 2, 2018

@sbennett77

This comment has been minimized.

sbennett77 commented Feb 5, 2018

Currently Testing.

@sbennett77

This comment has been minimized.

sbennett77 commented Feb 5, 2018

Passed.

@conklin94122

if (usernameSignature.isEmpty()) {
const QUuid& connectionToken = _connectionTokenHash.value(username.toLower());
if (usernameSignature.isEmpty() || connectionToken.isNull()) {

This comment has been minimized.

@birarda

birarda Feb 5, 2018

Member

This fixes a bug where the DS doesn't respond to subsequent check in requests from a user without permissions. In that case the user sends a packet with the previously used connection token, but the DS has already cleared that connection token from its list.

@@ -1046,7 +1046,8 @@ Application::Application(int& argc, char** argv, QElapsedTimer& startupTimer, bo
connect(nodeList.data(), &NodeList::packetVersionMismatch, this, &Application::notifyPacketVersionMismatch);
// you might think we could just do this in NodeList but we only want this connection for Interface
connect(nodeList.data(), &NodeList::limitOfSilentDomainCheckInsReached, nodeList.data(), &NodeList::reset);
connect(&nodeList->getDomainHandler(), SIGNAL(limitOfSilentDomainCheckInsReached()),
nodeList.data(), SLOT(reset()));

This comment has been minimized.

@huffman

huffman Feb 5, 2018

Member

is there a reason this was switched to use the old signal/slot style?

This comment has been minimized.

@birarda

birarda Feb 5, 2018

Member

The signature for reset now has a default argument that tells it wether or not to also reset the DomainHandler. The new signal/slot style isn't compatible with that.

This comment has been minimized.

@huffman

huffman Feb 5, 2018

Member

Ah, that makes sense.

// we haven't heard back from DS in MAX_SILENT_DOMAIN_SERVER_CHECK_INS
// so emit our signal that says that
qCDebug(networking) << "Limit of silent domain checkins reached";
emit limitOfSilentDomainCheckInsReached();

This comment has been minimized.

@birarda

birarda Feb 5, 2018

Member

I moved this logic to the DomainHandler since that seemed like a better place for it. Additionally I wanted it to reset the counter in processDomainServerConnectionDeniedPacket since that fixes a bug where we would reset the domain's connection even though we were hearing from the domain server.

@@ -255,7 +252,7 @@ void NodeList::reset() {
_avatarGainMap.clear();
_avatarGainMapLock.unlock();
if (sender() != &_domainHandler) {
if (!skipDomainHandlerReset) {

This comment has been minimized.

@birarda

birarda Feb 5, 2018

Member

I had to stop using the sender here because a reset from domain checkins no longer being responded to was moved to the DomainHandler.

@huffman

huffman approved these changes Feb 5, 2018

@birarda birarda merged commit 7613218 into highfidelity:master Feb 5, 2018

2 checks passed

default Build finished.
Details
license/cla Contributor License Agreement is signed.
Details

@birarda birarda deleted the birarda:bug/connection-refused branch Sep 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment