hellonode: Handle SIGINT, add hostname and \n to response #15
Conversation
- Ctrl+C on a Docker container sends SIGINT, which is not handled properly by default (see nodejs/node#4182). Added a signal handler. - Adding trailing newline to response for curl-friendliness. - Added log message indicating server is starting. - Added hostname to indicate that the container. Signed-off-by: Ahmet Alp Balkan <[email protected]>
hellonode/server.js
Outdated
|
|
||
| var handleRequest = function(request, response) { | ||
| console.log('Received request for URL: ' + request.url); | ||
| response.writeHead(200); | ||
| response.end('Hello, World!'); | ||
| response.end('Hello, World!\nHostname: ' + os.hostname() + '\n'); |
There was a problem hiding this comment.
you could use a template string here
`Hello, World!\nHostname: ${os.hostname()}\n`you could also potentially cache the hostname above to avoid the function calls on each request, that is perhaps overkill though
hellonode/server.js
Outdated
| @@ -15,11 +15,19 @@ | |||
| */ | |||
|
|
|||
| var http = require('http'); | |||
There was a problem hiding this comment.
modern node is using const rather than var for requires
hellonode/server.js
Outdated
| @@ -15,11 +15,19 @@ | |||
| */ | |||
|
|
|||
| var http = require('http'); | |||
| var os = require('os'); | |||
|
|
|||
There was a problem hiding this comment.
it might be nice to declare the port here, and give an option to declare it via env_var
const port = process.env.NODE_PORT || 8080;
hellonode/server.js
Outdated
|
|
||
| process.on('SIGINT', function() { | ||
| console.log('shutting down...'); | ||
| process.exit(); |
There was a problem hiding this comment.
if you SIGINT should the process exit cleanly?
hellonode/server.js
Outdated
| process.on('SIGINT', function() { | ||
| console.log('shutting down...'); | ||
| process.exit(); | ||
| }); | ||
|
|
||
| var handleRequest = function(request, response) { | ||
| console.log('Received request for URL: ' + request.url); |
There was a problem hiding this comment.
can be done with template string
console.log(`Received request for URL: ${request.url}`);
hellonode/server.js
Outdated
| var www = http.createServer(handleRequest); | ||
| console.log('server listening on port 8080'); |
There was a problem hiding this comment.
you might want to put this message inside of a callback to the listen call, to ensure it doesn't print until the server is listening
www.listen(port, ( ) => {
console.log(`server is listening on port ${port}`);
});Signed-off-by: Ahmet Alp Balkan <[email protected]>
| const http = require('http'); | ||
| const os = require('os'); | ||
|
|
||
| const port = process.env.PORT || 8080; |
There was a problem hiding this comment.
nit: do you want to make this more unique than PORT? possible to already exist on system.
There was a problem hiding this comment.
@MylesBorins kind of a convention, so I'll keep it.
properly by default (see Node v5.x.x does not exit on Ctrl-C in Docker container nodejs/node#4182). Added a signal handler.
Signed-off-by: Ahmet Alp Balkan [email protected]