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

Issue 579 Don't start heartbeat in createSocket #580

Merged
merged 4 commits into from May 1, 2017

Conversation

Projects
None yet
2 participants
@davedoesdev
Contributor

davedoesdev commented Apr 29, 2017

#579

Test uses process._getActiveHandles() to check setInterval() wasn't called.
Alternative would be to mock setInterval.

Show outdated Hide outdated index.js
@@ -1093,6 +1093,10 @@ Primus.prototype.reserved.events = {
*/
Primus.createSocket = function createSocket(options) {
options = options || {};
// Make sure the temporary Primus we create below doesn't start a heartbeat
options.pingInterval = 'pingInterval' in options

This comment has been minimized.

@lpinca

lpinca Apr 30, 2017

Member

Should we set it to false unconditionally? Can't think of a case where starting the heartbeat timer makes sense here.

@lpinca

lpinca Apr 30, 2017

Member

Should we set it to false unconditionally? Can't think of a case where starting the heartbeat timer makes sense here.

Show outdated Hide outdated test/transformer.base.js
pathname: server.pathname
})
, num_handles_after = process._getActiveHandles().length
, socket = new PSocket(server.addr);

This comment has been minimized.

@lpinca

lpinca Apr 30, 2017

Member

If I'm not wrong this is not needed for this test so I would remove it.

@lpinca

lpinca Apr 30, 2017

Member

If I'm not wrong this is not needed for this test so I would remove it.

@lpinca

lpinca approved these changes Apr 30, 2017

@davedoesdev

This comment has been minimized.

Show comment
Hide comment
@davedoesdev

davedoesdev Apr 30, 2017

Contributor

I've reworked the test to check setInterval isn't called rather than rely on the private function process._getActiveHandles.

I've also set pingInterval to false always. However, I'm modifying the passed-in options parameter which I'm a little uncomfortable with. I notice other places in index.js do this too. Wouldn't it be better to extend options instead of modifying what the caller passes in?

Contributor

davedoesdev commented Apr 30, 2017

I've reworked the test to check setInterval isn't called rather than rely on the private function process._getActiveHandles.

I've also set pingInterval to false always. However, I'm modifying the passed-in options parameter which I'm a little uncomfortable with. I notice other places in index.js do this too. Wouldn't it be better to extend options instead of modifying what the caller passes in?

@lpinca

This comment has been minimized.

Show comment
Hide comment
@lpinca

lpinca Apr 30, 2017

Member

Yes I agree, we should not mutate the original options object. There have been no complaints or bug reports though.

Member

lpinca commented Apr 30, 2017

Yes I agree, we should not mutate the original options object. There have been no complaints or bug reports though.

Show outdated Hide outdated index.js
@@ -1092,7 +1092,9 @@ Primus.prototype.reserved.events = {
* @api public
*/
Primus.createSocket = function createSocket(options) {
options = options || {};
options = Primus.prototype.merge.call(Primus, {}, options || {});

This comment has been minimized.

@lpinca

lpinca May 1, 2017

Member

We can also safely use Object.assign().

@lpinca

lpinca May 1, 2017

Member

We can also safely use Object.assign().

This comment has been minimized.

@davedoesdev

davedoesdev May 1, 2017

Contributor

Yes, I normally do that but didn't find it's use anywhere else in that file.

@davedoesdev

davedoesdev May 1, 2017

Contributor

Yes, I normally do that but didn't find it's use anywhere else in that file.

This comment has been minimized.

@lpinca

lpinca May 1, 2017

Member

It's used in the server code of the transformers (e.g.

this.service = new ws.Server(Object.assign({
) so feel free to use it.

@lpinca

lpinca May 1, 2017

Member

It's used in the server code of the transformers (e.g.

this.service = new ws.Server(Object.assign({
) so feel free to use it.

@lpinca lpinca merged commit 4b13b1b into primus:master May 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tejohnso tejohnso referenced this pull request Mar 30, 2018

Closed

Broken heartbeat protocol #646

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