Skip to content

Mimic Dash error if running app on port already in use #79

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Mimic Dash error if running app on port already in use #79

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1522f7e
mimic dash error if running app on port already in use
jayceslesar Jun 19, 2022
ea4c3aa
localhost -> self._host
jayceslesar Jun 20, 2022
0d788d1
raise error instead of exit
jayceslesar Jun 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions plotly_resampler/figure_resampler/figure_resampler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

__author__ = "Jonas Van Der Donckt, Jeroen Van Der Donckt, Emiel Deprost"

import sys
import socket
import warnings
from typing import Tuple

Expand Down Expand Up @@ -160,6 +162,15 @@ def show_dash(
self._host = kwargs.get("host", "127.0.0.1")
self._port = kwargs.get("port", "8050")

# check if self._port is already in use
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
if s.connect_ex(('localhost', int(self._port))) == 0:
error_message = "Address already in use\n" \
f"Port {self._port} is in use by another program. " \
"Either identify and stop that program, or start the server with a different port."
print(error_message)
Copy link
Member

@jonasvdd jonasvdd Jun 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why is print + sys.exit used? Isn't it clearer to raise an error with that error message?

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 think you are right here, but when I run a dash app twice on the same host and port I dont get an error message thrown, it just tells me the same message above and quits. Not sure if this is within flask or dash but would be good to check that against those two as well as fastapi. Definitely agree that swallowing the error and quitting isnt ideal, so maybe an upstream change could be welcome here wherever that implementation lies.

Another test I need to make is if dash has this same behavior in a notebook as we are seeing here (taking too long to let the user know that the port is already in use)

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 think the other real reason I dont want to throw an error here is that this is just calling the upstream libraries and honestly shouldnt be this library's problem to catch that imo as all it does is resample really well. I will do some digging at some point today to confirm any assumptions in the above comment

Copy link
Member

Choose a reason for hiding this comment

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

I'll have a look myself later this week (I guess we should look at dash & jupyter dash) :)

Would be amazing if this PR avoids the cases where it takes dash way too long to notify the users when the port is already used!

Copy link
Member

Choose a reason for hiding this comment

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

Okay, just validated this PR! Seems to work as expected 🔥

My 5 cents about sys.exit vs raising an error; although throwing an error in these cases is not at all the responsibility of this library, I think calling sys.exit(0) is arguable even worse. Hence, I would propose to raise an exception (OSError, with your error message - perhaps you could inlude self._host in the msg as well) - as this exception gets raised a lot quicker than the upstream exceptions we are talking about. Do you agree with this @jayceslesar?

In an ideal world we would fix the issue / weird behavior in the upstream library (but I do not seem to find quickly where these issues stem from)

Copy link
Contributor Author

@jayceslesar jayceslesar Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I raised the error and for reference here is where it is implemented in werkzeug, which is the error we were/are seeing with this change https://github.com/pallets/werkzeug/blob/edef71c243e1e1396092f7b5f82ddad6c6f766cf/src/werkzeug/serving.py#L907

sys.exit(1)

app.run_server(mode=mode, **kwargs)

def stop_server(self, warn: bool = True):
Expand Down