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

Add listening method to net + tests #4743

Closed
wants to merge 1 commit into from

Conversation

cusspvz
Copy link
Contributor

@cusspvz cusspvz commented Jan 18, 2016

Added:

  • listening method into net
  • Added test for net module
  • Added test for http module, since it inherits properties from net

closes #4735

@r-52 r-52 added http Issues or PRs related to the http subsystem. net Issues and PRs related to the net subsystem. and removed http Issues or PRs related to the http subsystem. labels Jan 18, 2016
@mscdex
Copy link
Contributor

mscdex commented Jan 18, 2016

I think a boolean property (either normal or a getter) might be a better fit for this.

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 18, 2016
@cusspvz
Copy link
Contributor Author

cusspvz commented Jan 18, 2016

@mscdex deal. Gonna change it.

@jasnell
Copy link
Member

jasnell commented Jan 18, 2016

This is fine. Just a thought but setting this up as a read-only property instead of a method might be worthwhile... e.g

Object.defineProperty(Server.prototype, 'listening', {
  configurable: false,
  enumerable: true,
  get : function() {
    return !! this._handle;
  }
});

Another thing, @cusspvz ... would you be able to update the commit log to match the style guidelines outlined here

@cjihrig
Copy link
Contributor

cjihrig commented Jan 18, 2016

Can you also run make jslint.

@@ -0,0 +1,13 @@
'use strict';

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This empty line is not needed.

@cusspvz cusspvz changed the title Add listening method to net + tests [WIP] Add listening method to net + tests Jan 18, 2016
get: function() {
return !! self._handle;
},
set: function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd simply omit the set here. There's no reason to add a new throw as this can easily be marked as read only in the documentation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having a setter that throws, maybe just make writable: false

@jasnell
Copy link
Member

jasnell commented Jan 18, 2016

Thank you for bearing with us :-) It's looking good. One other thing this will need is documentation for the new property to be added to /doc/api/net.markdown

'Use .listen() or .close() instead.'
)
}
configurable: true, enumerable: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you split these across lines?

Also, why is enumerable not true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed to true already. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should i stick configurable as true as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't personally see a need to make it configurable.

@cusspvz
Copy link
Contributor Author

cusspvz commented Jan 18, 2016

@jasnell

I've setup property definition into the Server constructor because I was doubting if it would pass the right context if it is defined on it's property.

Object.defineProperty(Server.prototype, 'listening', {
  get : function() {
    return !! this._handle;
  },
  configurable: false, enumerable: true,
});

Does that work?

@jasnell
Copy link
Member

jasnell commented Jan 18, 2016

It doesn't need to go into the constructor. When the getter function is invoked it will be passed the appropriate this. If you do the defineProperty within the constructor however, you should change the first argument from Server.prototype to this, but I would just leave it outside of the constructor.

@cusspvz cusspvz changed the title [WIP] Add listening method to net + tests Add listening method to net + tests Jan 18, 2016
@cusspvz
Copy link
Contributor Author

cusspvz commented Jan 18, 2016

Think I did all requested changes. I just have to run the make jslint command. I will let you know how it went after my coffee break. Thank you all for the tips! :)

let common = require('../common');
let assert = require('assert');
let http = require('http');

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should be const :-)

@jasnell
Copy link
Member

jasnell commented Jan 18, 2016

Looking good, almost there! I left one more comment. I'll run CI once that's addressed

@@ -548,6 +548,11 @@ parameter is 511 (not 512).
This function is asynchronous. The last parameter `callback` will be added as
a listener for the `'listening'` event. See also [`net.Server.listen(port)`][].

### server.listening

Returns a boolean telling if the server is whether or not listening for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Returns a Boolean indicating whether or not the server is listening for connections."

@cjihrig
Copy link
Contributor

cjihrig commented Jan 18, 2016

This could probably use additional tests to at least verify that the value is false after a close().

@cusspvz
Copy link
Contributor Author

cusspvz commented Jan 21, 2016

@evanlucas looks good?

evanlucas pushed a commit that referenced this pull request Jan 21, 2016
Added a listening property into net.Server.prototype indicating
if the server is listening or not for connections.

Other Server constructors that rely on net.Server should also
gain access to this property.

Also included tests for net and http subsystems.

PR-URL: #4743
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@evanlucas
Copy link
Contributor

Landed in 5ef9989. Thanks for the contribution @cusspvz!!

@evanlucas evanlucas closed this Jan 21, 2016
@cusspvz
Copy link
Contributor Author

cusspvz commented Jan 22, 2016

You're welcome @evanlucas . Would this be merged also on current LTS versions? Thanks :)

@evanlucas
Copy link
Contributor

I don't see it landing on v4.x unless another semver-minor version is going out for LTS, but do see it landing in v5.x.

@jasnell
Copy link
Member

jasnell commented Jan 23, 2016

yeah, the general approach is to avoid landing semver-minor or higher in LTS tho there's going to be a rare exception here soon to land a couple of v8 profiling improvements. While I'm very happy to see this land, I'm not sure it rises to the level of something we'd land in v4 at this point.

@cusspvz
Copy link
Contributor Author

cusspvz commented Jan 23, 2016

Just asked because I have some modules where I'm gonna use this feature, and they must have compability with LTS versions.

But I was guessing, seems right, since this is not a patch. Thanks once again! :)

@jasnell
Copy link
Member

jasnell commented Jan 23, 2016

understood... and something @nodejs/lts should keep in mind. It ought to be something that's fairly simple to polyfill to ensure the API use is consistent tho. I know that's not an overly helpful answer tho

rvagg pushed a commit that referenced this pull request Feb 10, 2016
Added a listening property into net.Server.prototype indicating
if the server is listening or not for connections.

Other Server constructors that rely on net.Server should also
gain access to this property.

Also included tests for net and http subsystems.

PR-URL: #4743
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
rvagg added a commit that referenced this pull request Feb 21, 2016
* buffer:
  - You can now supply an encoding argument when filling a
    Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
    an existing Buffer will also work with
    Buffer#fill(buffer[, start[, end]]). See the API documentation for
    details on how this works. (Trevor Norris) #4935
  - Buffer#indexOf() no longer requires a byteOffset argument if you
    also wish to specify an encoding:
    Buffer#indexOf(val[, byteOffset][, encoding]).
    (Trevor Norris) #4803
* child_process: spawn() and spawnSync() now support a 'shell' option
  to allow for optional execution of the given command inside a shell.
  If set to true, cmd.exe will be used on Windows and /bin/sh
  elsewhere. A path to a custom shell can also be passed to override
  these defaults. On Windows, this option allows .bat. and .cmd files
  to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
  strict limitation of allowable header characters.
  (James M Snell) #5237
* dgram: socket.send() now supports accepts an array of Buffers or
  Strings as the first argument. See the API docs for details on how
  this works. (Matteo Collina) #4374
* http: Fix a bug where handling headers will mistakenly trigger an
  'upgrade' event where the server is just advertising its protocols.
  This bug can prevent HTTP clients from communicating with HTTP/2
  enabled servers. (Fedor Indutny) #4337
* net: Added a listening Boolean property to net and http servers to
  indicate whether the server is listening for connections.
  (José Moreira) #4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
  it from inside another MakeCallback() call no longer causes the
  nextTick queue or Promises microtask queue to be processed out of
  order. (Trevor Norris) #4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
  TLS protocol version of the current connection. (Brian White) #4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
  new vm.Script() to interact with V8's code cache. When a new
  vm.Script object is created with the 'produceCachedData' set to true
  a Buffer with V8's code cache data will be produced and stored in
  cachedData property of the returned object. This data in turn may be
  supplied back to another vm.Script() object with a 'cachedData'
  option if the supplied source is the same. Successfully executing a
  script from cached data can speed up instantiation time. See the API
  docs for details. (Fedor Indutny) #4777
* performance: Improvements in:
  - process.nextTick() (Ruben Bridgewater) #5092
  - path module (Brian White) #5123
  - querystring module (Brian White) #5012
  - streams module when processing small chunks (Matteo Collina) #4354
rvagg added a commit that referenced this pull request Feb 21, 2016
* buffer:
  - You can now supply an encoding argument when filling a
    Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
    an existing Buffer will also work with
    Buffer#fill(buffer[, start[, end]]). See the API documentation for
    details on how this works. (Trevor Norris) #4935
  - Buffer#indexOf() no longer requires a byteOffset argument if you
    also wish to specify an encoding:
    Buffer#indexOf(val[, byteOffset][, encoding]).
    (Trevor Norris) #4803
* child_process: spawn() and spawnSync() now support a 'shell' option
  to allow for optional execution of the given command inside a shell.
  If set to true, cmd.exe will be used on Windows and /bin/sh
  elsewhere. A path to a custom shell can also be passed to override
  these defaults. On Windows, this option allows .bat. and .cmd files
  to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
  strict limitation of allowable header characters.
  (James M Snell) #5237
* dgram: socket.send() now supports accepts an array of Buffers or
  Strings as the first argument. See the API docs for details on how
  this works. (Matteo Collina) #4374
* http: Fix a bug where handling headers will mistakenly trigger an
  'upgrade' event where the server is just advertising its protocols.
  This bug can prevent HTTP clients from communicating with HTTP/2
  enabled servers. (Fedor Indutny) #4337
* net: Added a listening Boolean property to net and http servers to
  indicate whether the server is listening for connections.
  (José Moreira) #4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
  it from inside another MakeCallback() call no longer causes the
  nextTick queue or Promises microtask queue to be processed out of
  order. (Trevor Norris) #4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
  TLS protocol version of the current connection. (Brian White) #4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
  new vm.Script() to interact with V8's code cache. When a new
  vm.Script object is created with the 'produceCachedData' set to true
  a Buffer with V8's code cache data will be produced and stored in
  cachedData property of the returned object. This data in turn may be
  supplied back to another vm.Script() object with a 'cachedData'
  option if the supplied source is the same. Successfully executing a
  script from cached data can speed up instantiation time. See the API
  docs for details. (Fedor Indutny) #4777
* performance: Improvements in:
  - process.nextTick() (Ruben Bridgewater) #5092
  - path module (Brian White) #5123
  - querystring module (Brian White) #5012
  - streams module when processing small chunks (Matteo Collina) #4354
rvagg added a commit that referenced this pull request Feb 23, 2016
* buffer:
  - You can now supply an encoding argument when filling a
    Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
    an existing Buffer will also work with
    Buffer#fill(buffer[, start[, end]]). See the API documentation for
    details on how this works. (Trevor Norris) #4935
  - Buffer#indexOf() no longer requires a byteOffset argument if you
    also wish to specify an encoding:
    Buffer#indexOf(val[, byteOffset][, encoding]).
    (Trevor Norris) #4803
* child_process: spawn() and spawnSync() now support a 'shell' option
  to allow for optional execution of the given command inside a shell.
  If set to true, cmd.exe will be used on Windows and /bin/sh
  elsewhere. A path to a custom shell can also be passed to override
  these defaults. On Windows, this option allows .bat. and .cmd files
  to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
  strict limitation of allowable header characters.
  (James M Snell) #5237
* dgram: socket.send() now supports accepts an array of Buffers or
  Strings as the first argument. See the API docs for details on how
  this works. (Matteo Collina) #4374
* http: Fix a bug where handling headers will mistakenly trigger an
  'upgrade' event where the server is just advertising its protocols.
  This bug can prevent HTTP clients from communicating with HTTP/2
  enabled servers. (Fedor Indutny) #4337
* net: Added a listening Boolean property to net and http servers to
  indicate whether the server is listening for connections.
  (José Moreira) #4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
  it from inside another MakeCallback() call no longer causes the
  nextTick queue or Promises microtask queue to be processed out of
  order. (Trevor Norris) #4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
  TLS protocol version of the current connection. (Brian White) #4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
  new vm.Script() to interact with V8's code cache. When a new
  vm.Script object is created with the 'produceCachedData' set to true
  a Buffer with V8's code cache data will be produced and stored in
  cachedData property of the returned object. This data in turn may be
  supplied back to another vm.Script() object with a 'cachedData'
  option if the supplied source is the same. Successfully executing a
  script from cached data can speed up instantiation time. See the API
  docs for details. (Fedor Indutny) #4777
* performance: Improvements in:
  - process.nextTick() (Ruben Bridgewater) #5092
  - path module (Brian White) #5123
  - querystring module (Brian White) #5012
  - streams module when processing small chunks (Matteo Collina) #4354

PR-URL: #5295
rvagg added a commit that referenced this pull request Feb 24, 2016
* buffer:
  - You can now supply an encoding argument when filling a
    Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
    an existing Buffer will also work with
    Buffer#fill(buffer[, start[, end]]). See the API documentation for
    details on how this works. (Trevor Norris) #4935
  - Buffer#indexOf() no longer requires a byteOffset argument if you
    also wish to specify an encoding:
    Buffer#indexOf(val[, byteOffset][, encoding]).
    (Trevor Norris) #4803
* child_process: spawn() and spawnSync() now support a 'shell' option
  to allow for optional execution of the given command inside a shell.
  If set to true, cmd.exe will be used on Windows and /bin/sh
  elsewhere. A path to a custom shell can also be passed to override
  these defaults. On Windows, this option allows .bat. and .cmd files
  to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
  strict limitation of allowable header characters.
  (James M Snell) #5237
* dgram: socket.send() now supports accepts an array of Buffers or
  Strings as the first argument. See the API docs for details on how
  this works. (Matteo Collina) #4374
* http: Fix a bug where handling headers will mistakenly trigger an
  'upgrade' event where the server is just advertising its protocols.
  This bug can prevent HTTP clients from communicating with HTTP/2
  enabled servers. (Fedor Indutny) #4337
* net: Added a listening Boolean property to net and http servers to
  indicate whether the server is listening for connections.
  (José Moreira) #4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
  it from inside another MakeCallback() call no longer causes the
  nextTick queue or Promises microtask queue to be processed out of
  order. (Trevor Norris) #4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
  TLS protocol version of the current connection. (Brian White) #4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
  new vm.Script() to interact with V8's code cache. When a new
  vm.Script object is created with the 'produceCachedData' set to true
  a Buffer with V8's code cache data will be produced and stored in
  cachedData property of the returned object. This data in turn may be
  supplied back to another vm.Script() object with a 'cachedData'
  option if the supplied source is the same. Successfully executing a
  script from cached data can speed up instantiation time. See the API
  docs for details. (Fedor Indutny) #4777
* performance: Improvements in:
  - process.nextTick() (Ruben Bridgewater) #5092
  - path module (Brian White) #5123
  - querystring module (Brian White) #5012
  - streams module when processing small chunks (Matteo Collina) #4354

PR-URL: #5295
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Added a listening property into net.Server.prototype indicating
if the server is listening or not for connections.

Other Server constructors that rely on net.Server should also
gain access to this property.

Also included tests for net and http subsystems.

PR-URL: nodejs#4743
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@pvsousalima pvsousalima mentioned this pull request Nov 1, 2016
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Add a public API constant to net/http/https Server telling whereas the server is listening or not
10 participants