Skip to content
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

Unauthenticated connection will unregister the current connected one #337

Merged
merged 10 commits into from
Jan 5, 2020

Conversation

seeLuck
Copy link
Contributor

@seeLuck seeLuck commented Dec 2, 2019

This commit fix bug:

Two connection with same client id. Connection A passed authenticate and connect to broker successfully. Connection B reject by authenticate and failed to connect to broker.

While with this failed Connection B, the current client info will be unregisted in broker.

Few lines to fix it.

@seeLuck
Copy link
Contributor Author

seeLuck commented Dec 2, 2019

The CI failed with not ok 333 one connected clients.

But it is not my case. The master branch also failed with this one.

@mcollina
Copy link
Collaborator

mcollina commented Dec 2, 2019

Would you mind adding a unit test?

--- We'll need a fix for the failing test as well.

@seeLuck
Copy link
Contributor Author

seeLuck commented Dec 3, 2019

done :)
@mcollina

@robertsLando
Copy link
Member

robertsLando commented Dec 18, 2019

@seeLuck there is one test that is failing

not ok 335 one connected clients
  ---
    operator: equal
    expected: 1
    actual:   2
    at: <anonymous> (/home/travis/build/mcollina/aedes/test/meta.js:30:11)
    stack: |-
      Error: one connected clients
          at Test.assert [as _assert] (/home/travis/build/mcollina/aedes/node_modules/tape/lib/test.js:225:54)
          at Test.bound [as _assert] (/home/travis/build/mcollina/aedes/node_modules/tape/lib/test.js:77:32)
          at Test.equal (/home/travis/build/mcollina/aedes/node_modules/tape/lib/test.js:385:10)
          at Test.bound [as equal] (/home/travis/build/mcollina/aedes/node_modules/tape/lib/test.js:77:32)
          at /home/travis/build/mcollina/aedes/test/meta.js:30:11
          at _combinedTickCallback (internal/process/next_tick.js:132:7)
          at process._tickCallback (internal/process/next_tick.js:181:9)
  ...

@seeLuck
Copy link
Contributor Author

seeLuck commented Dec 19, 2019

@robertsLando
It seems not my problem, I just merge the branch and all passed.

@robertsLando
Copy link
Member

robertsLando commented Dec 19, 2019 via email

@seeLuck
Copy link
Contributor Author

seeLuck commented Jan 3, 2020

@mcollina
Hi boss, how about merge this bugfix commit and update npm package? Thx a lot.

@mcollina mcollina merged commit 836d5b7 into moscajs:master Jan 5, 2020
@mcollina
Copy link
Collaborator

mcollina commented Jan 5, 2020

Landed, I hope I can get a new release out soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants