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

Make sure Accounts login only sets 1 onReconnect callback #9148

Merged
merged 1 commit into from Sep 28, 2017

Conversation

Projects
None yet
2 participants
@hwillson
Member

hwillson commented Sep 28, 2017

The changes added in d854a4b fixed a long standing issue where the Accounts system was overwriting other DDP connection.onReconnect callbacks, that were potentially set by developers (and vice-versa - developers could overwrite the onReconnect callback registered by the Accounts system, which impacted logging back in after reconnecting). Unfortunately these changes are also registering a new duplicate onReconnect callback to be called, after every login. These duplicate callbacks pile up and are all called when reconnecting, which eventually breaks user logins.

The changes in this commit make sure that any previously set Accounts login onReconnect callback is first removed, before adding a new callback. This ensures the Accounts system is only ever setting one onReconnect callback after logging in.

Fixes #9140.

Make sure Accounts login only sets 1 onReconnect callback
The changes added in
d854a4b
fixed a long standing issue where the Accounts system was
overwriting other DDP `connection.onReconnect` callbacks,
that were potentially set by developers (and vice-versa -
developers could overwrite the `onReconnect` callback registered
by the Accounts system, which impacted logging back in after
reconnecting). Unfortunately these changes are also registering a
new duplicate `onReconnect` callback to be called, after every
login. These duplicate callbacks pile up and are all called when
reconnecting, which eventually breaks user logins.

The changes in this commit make sure that any previously set
Accounts login `onReconnect` callback is first removed, before
adding a new callback. This ensures the Accounts system is only
ever setting one `onReconnect` callback after logging in.

Fixes #9140.

@hwillson hwillson requested review from benjamn and abernix Sep 28, 2017

@hwillson

This comment has been minimized.

Member

hwillson commented Sep 28, 2017

@abernix @benjamn The test failures are likely due to the new dev_bundle missing (that needs to be published as part of #9137).

abernix referenced this pull request Sep 28, 2017

Add DDP.onReconnect(), deprecate conn.onReconnect (#9092)
* Fix bug #5665: add DDP.onReconnect(), deprecate conn.onReconnect

Deprecate use of `connection.onReconnect = func`. Instead, a new
`DDP.onReconnect(callback)` method should be used to register callbacks to call
when a connection reconnects. The connection that is reconnecting is passed as
the only argument to `callback`. This is used by the accounts system to relogin
on reconnects without interfering with other code which uses
`connection.onReconnect`.

* Adjust History entry, package versions, code cleanup

@abernix abernix changed the base branch from devel to release-1.5.2.2 Sep 28, 2017

@abernix abernix added this to the 1.5.2.2 milestone Sep 28, 2017

@abernix abernix merged commit 01b5487 into meteor:release-1.5.2.2 Sep 28, 2017

1 of 3 checks passed

ci/circleci: Get Ready Your tests failed on CircleCI
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
CLA Author has signed the Meteor CLA.
Details
@abernix

This comment has been minimized.

Member

abernix commented Sep 28, 2017

Merged into release-1.5.2.2 where the dev bundle has been rebuilt. We will await the test results on that branch (in progress now).

@abernix

This comment has been minimized.

Member

abernix commented Sep 28, 2017

Just confirming that the tests for the merge commit (01b5487) and passed : https://circleci.com/workflow-run/25eda4df-3993-4611-91fa-4e7f51d2b9c5.

Thanks so much for the fix and amended test, @hwillson!

@abernix

This comment has been minimized.

Member

abernix commented Sep 28, 2017

A release candidate for Meteor 1.5.2.2 has been released which includes this change. Please help test it and confirm whether this issue is fixed!

meteor update --release 1.5.2.2-rc.0

@MartinSchoeler MartinSchoeler referenced this pull request Oct 2, 2017

Closed

Automatic logouts in 0.58.3/Facebook #8366

0 of 3 tasks complete

@edemaine edemaine referenced this pull request Oct 4, 2017

Closed

Coauthor logging out #252

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