fix: offer a new port and listen if already used, use consola on server error #4428

ricardogobbosouza · 2018-11-26T19:50:55Z

Types of changes

Bug fix (a non-breaking change which fixes an issue)
New feature (a non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Description

If port is already in use, offer a new and listen
Use consola on server error

Resolves: #4421

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly. (PR: #)
I have added tests to cover my changes (if not applicable, please state why)
All new and existing tests are passing.

codecov-io · 2018-11-26T19:57:19Z

Codecov Report

Merging #4428 into dev will increase coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             dev    #4428      +/-   ##
=========================================
+ Coverage   91.4%   91.48%   +0.08%     
=========================================
  Files         56       56              
  Lines       1943     1962      +19     
  Branches     485      487       +2     
=========================================
+ Hits        1776     1795      +19     
  Misses       155      155              
  Partials      12       12

Impacted Files	Coverage Δ
packages/server/src/server.js	`100% <ø> (ø)`	⬆️
packages/server/src/listener.js	`98.11% <100%> (+1.05%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee13925...6e10375. Read the comment docs.

manniL · 2018-11-27T00:35:23Z

Before

Current (after that PR)

consola.fatal will actually terminate the process because of https://github.com/nuxt/nuxt.js/blob/7c4e77ffb9d18279bd9d0c5f4d39eb55569c6a81/packages/cli/src/setup.js#L25-L42. So we might should roll with consola.error(e)

With suggested changes

galvez

Need to change to consola.error() like @manniL suggested but otherwise LGTM!

Atinux · 2018-11-27T15:31:46Z

I think we can improve more the DX on this.

Why not checking the ECODE and display a better error instead when we know it?

manniL · 2018-11-27T15:43:23Z

@Atinux Had the same idea.

@ricardogobbosouza Do you want to take that or should we?

ricardogobbosouza · 2018-11-27T17:51:06Z

@manniL, @Atinux, I thought of something like this:

this._server.on('error', (e) => {
  if (e.code === 'EADDRINUSE') {
    return consola.error(`Address already in use \`${this.host}:${this.port}\``)
  }

  consola.error(e)
}

What do you think?

manniL · 2018-11-27T18:10:01Z

@ricardogobbosouza Maybe like

Address \`${this.host}:${this.port}\` is already in use. Do you run another service on the same port?

We could do that for more common codes as well (list of all: http://man7.org/linux/man-pages/man3/errno.3.html)

ricardogobbosouza · 2018-11-27T19:48:39Z

What will we customize? Everyone would be unnecessary, do not you think?

manniL · 2018-11-27T20:36:54Z

@ricardogobbosouza Not every single one ☺️
Maybe EDQUOT and EACCES as well?
What do you think @pi0? ☺️

pi0 · 2018-11-27T21:26:46Z

@manniL @ricardogobbosouza V8 error codes are listed here. Honestly, I would like to add this as a feature into Consola to translate error.codes. This PR is already good as is.

Something else as a suggestion: For development mode, like other frameworks (CRA, Next,..) why not gently offer a new port and listen on it? :D

ricardogobbosouza · 2018-11-27T22:21:56Z

@pi0 I fully agree that the error handling is done in the consola and that a new port should be suggested to the user.
I'm making this improvement...

This reverts commit 770347a.

This reverts commit 26f2588.

HugoHeneault · 2018-11-28T20:30:13Z

@manniL Nice idea. But won't it be annoying to change ports at each nuxt restart? We need to restart it when we change nuxt.config.js and so we should update our tabs port? Would be nicer to take 3001 if 3000 is already taken, then 3002, etc.

manniL · 2018-11-28T20:48:21Z

@HugoHeneault Well, get-port would also choose a random port 🤷‍♂️
But I see your point there 🤔

ricardogobbosouza · 2018-11-28T22:32:29Z

@HugoHeneault I think a random port is better than creating a counter for it.
Because if the defined port is already in use, the consola will warn.

ricardogobbosouza · 2018-11-29T10:04:47Z

@pi0 @manniL @Atinux
What do you think? Better to put on an counter?
For me a random port resolves well

manniL · 2018-11-29T10:20:56Z

I'm fine with a random port if the initial one is blocked 👍

pi0 · 2018-11-29T17:43:36Z

packages/server/src/listener.js

+
+  async serverErrorHandler(e) {
+    if (e.code === 'EADDRINUSE') {
+      consola.warn(`Address \`${this.host}:${this.port}\` is already in use.`)


For production, we have to throw a FATAL error because the server is not usable and we silently continue listening on a random port. This makes production debug hard

pi0 · 2018-11-29T19:26:46Z

@ricardogobbosouza I've made some improvement. Would you please review latest diff?

pi0 · 2018-11-29T19:29:35Z

packages/server/src/listener.js

+    }
+
+    // Throw error
+    throw error


Tip: This error is handled by consola when using cli. The reason why not using consola here is that programatic usage needs to capture the listen error instead of silently skipping

pi0 · 2018-11-29T20:23:36Z

Dev:

Prod:

ricardogobbosouza · 2018-11-29T22:53:54Z

@pi0 nice!! 👍

manniL · 2018-11-29T23:24:31Z

🔥 🔥 🔥

lock · 2018-12-30T16:05:23Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Use consola on server error

69db642

pi0 requested a review from manniL November 26, 2018 19:51

fix style

efcdf43

ignore coverage

f9d5457

galvez previously approved these changes Nov 27, 2018

View reviewed changes

use consola.error(e)

21eda65

ricardogobbosouza dismissed galvez’s stale review via 21eda65 November 27, 2018 01:36

galvez assigned ricardogobbosouza Nov 27, 2018

galvez previously approved these changes Nov 27, 2018

View reviewed changes

formatting server error

1113b5d

ricardogobbosouza dismissed galvez’s stale review via 1113b5d November 27, 2018 20:52

fix style

96df317

ricardogobbosouza added 5 commits November 27, 2018 20:52

offer a new port and listen

22a4f60

fix style

770347a

simplify return

26f2588

Revert "fix style"

e3a0830

This reverts commit 770347a.

Revert "simplify return"

332a7e3

This reverts commit 26f2588.

ricardogobbosouza changed the title ~~fix: use consola on server error~~ feat: offer a new port and listen, use consola on server error Nov 27, 2018

ricardogobbosouza changed the title ~~feat: offer a new port and listen, use consola on server error~~ fix: offer a new port and listen if already used, use consola on server error Nov 27, 2018

simplified tests

1fa4d4f

manniL mentioned this pull request Nov 28, 2018

consola v3 proposal unjs/consola#30

Closed

update this.port value with address.port

73be36c

pi0 reviewed Nov 29, 2018

View reviewed changes

ricardogobbosouza and others added 6 commits November 29, 2018 16:14

For production, use consola.fatal

ab883b7

pass options.dev from server to listener constructor

f57f8b9

add dev on constructor

a3b001d

improve serverErrorHandler and close

e35dfa6

Update listener.js

833425c

improve serverErrorHandler

90050e0

pi0 reviewed Nov 29, 2018

View reviewed changes

pi0 added 3 commits November 29, 2018 23:45

improve the way to handle listen errors

3fc7f4d

fix missed line

2b9df24

fully close old server before listening on a random port

820b962

update listen.test

6e10375

manniL approved these changes Nov 29, 2018

View reviewed changes

Atinux merged commit 1d78027 into nuxt:dev Nov 30, 2018

galvez mentioned this pull request Dec 17, 2018

Return random port in nuxt.options.server #4213

Closed

lock bot locked as resolved and limited conversation to collaborators Dec 30, 2018

danielroe added the 2.x label Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: offer a new port and listen if already used, use consola on server error #4428

fix: offer a new port and listen if already used, use consola on server error #4428

ricardogobbosouza commented Nov 26, 2018 •

edited

codecov-io commented Nov 26, 2018 •

edited

manniL commented Nov 27, 2018 •

edited

galvez left a comment

Atinux commented Nov 27, 2018

manniL commented Nov 27, 2018

ricardogobbosouza commented Nov 27, 2018

manniL commented Nov 27, 2018

ricardogobbosouza commented Nov 27, 2018

manniL commented Nov 27, 2018

pi0 commented Nov 27, 2018

ricardogobbosouza commented Nov 27, 2018

HugoHeneault commented Nov 28, 2018 •

edited

manniL commented Nov 28, 2018

ricardogobbosouza commented Nov 28, 2018

ricardogobbosouza commented Nov 29, 2018

manniL commented Nov 29, 2018

pi0 Nov 29, 2018

pi0 commented Nov 29, 2018

pi0 Nov 29, 2018 •

edited

pi0 commented Nov 29, 2018

ricardogobbosouza commented Nov 29, 2018

manniL commented Nov 29, 2018

lock bot commented Dec 30, 2018

fix: offer a new port and listen if already used, use consola on server error #4428

fix: offer a new port and listen if already used, use consola on server error #4428

Conversation

ricardogobbosouza commented Nov 26, 2018 • edited

Types of changes

Description

Checklist:

codecov-io commented Nov 26, 2018 • edited

Codecov Report

manniL commented Nov 27, 2018 • edited

Before

Current (after that PR)

With suggested changes

galvez left a comment

Choose a reason for hiding this comment

Atinux commented Nov 27, 2018

manniL commented Nov 27, 2018

ricardogobbosouza commented Nov 27, 2018

manniL commented Nov 27, 2018

ricardogobbosouza commented Nov 27, 2018

manniL commented Nov 27, 2018

pi0 commented Nov 27, 2018

ricardogobbosouza commented Nov 27, 2018

HugoHeneault commented Nov 28, 2018 • edited

manniL commented Nov 28, 2018

ricardogobbosouza commented Nov 28, 2018

ricardogobbosouza commented Nov 29, 2018

manniL commented Nov 29, 2018

pi0 Nov 29, 2018

Choose a reason for hiding this comment

pi0 commented Nov 29, 2018

pi0 Nov 29, 2018 • edited

Choose a reason for hiding this comment

pi0 commented Nov 29, 2018

ricardogobbosouza commented Nov 29, 2018

manniL commented Nov 29, 2018

lock bot commented Dec 30, 2018

ricardogobbosouza commented Nov 26, 2018 •

edited

codecov-io commented Nov 26, 2018 •

edited

manniL commented Nov 27, 2018 •

edited

HugoHeneault commented Nov 28, 2018 •

edited

pi0 Nov 29, 2018 •

edited