Mimic Dash error if running app on port already in use

[jayceslesar]

closes #73.

In Dash if you try to do this you get an output like:

Address already in use
Port 8080 is in use by another program. Either identify and stop that program, or start the server with a different port.

So we mimic that here.

[jonasvdd]

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

[jayceslesar]

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)

[jayceslesar]

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

[jvdd]

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!

[jvdd]

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)

[jayceslesar]

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

[jonasvdd]

Hi @jayceslesar!

Thanks for contributing, looks promising! 👏🏼
I did not immediately find the dash implementation; could you provide a link to which snippet you based this implementation on?

TODO:

• Review and compare with dash implementation
• Add tests

[codecov-commenter]

Codecov Report

Merging #79 (ea4c3aa) into main (f25cc4e) will decrease coverage by 0.38%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##             main      #79      +/-   ##
==========================================
- Coverage   98.11%   97.73%   -0.39%     
==========================================
  Files          10       10              
  Lines         743      750       +7     
==========================================
+ Hits          729      733       +4     
- Misses         14       17       +3     

Impacted Files	Coverage Δ
...tly_resampler/figure_resampler/figure_resampler.py	86.56% <57.14%> (-3.44%)	⬇️

---

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 f25cc4e...ea4c3aa. Read the comment docs.

[jayceslesar]

also looks like this is the second time the failing test has failed on different python versions while the other two passed haha. Not a stranger to those kinds of tests

[jvdd]

Hi @jayceslesar

I did some further digging today and found out why it takes (quite) some time before jupyter-dash throws an OSError when the address is already in use. In the run method multiple times they check if an alive message from the address can be received (see code below)

https://github.com/plotly/jupyter-dash/blob/b6dad2689af96b70b796ae3d766a9c56d306c47d/jupyter_dash/jupyter_app.py#L323-L342

As this apparently intended behavior of jupyter-dash, I'm not really keen on overwriting this in this library (as it is not at all our responsibility).

What do you think of this? Or were you targeting another bug / issue with this PR (that I am missing at the moment)?

Cheers, Jeroen

[jayceslesar]

I think taking no action is a fine way to deal with this -- I completely agree that it is not this libraries job to better handle how things are treated upstream. Feel free to close

[jayceslesar]

Good to close this and #73 ?

[jonasvdd]

I experienced (plotly-resampler 0.8.0) that this dash app error might keep "hanging" indefinitely, seems to be caused by a dash-werkzeug combination. Will need to further look into this (fyi: @jvdd @jayceslesar )

plotly/jupyter-dash#103 (comment)

Will create a new issue for this later this day.