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

Check port availability #4333

Merged
merged 3 commits into from
Oct 3, 2018

Conversation

lyw07
Copy link
Contributor

@lyw07 lyw07 commented Sep 26, 2018

Summary

This PR is to fix the issue that the function for port availability check does not really work. Previously, because of the wrong host, the unavailable port error is not caught by the function check_port_availability but by cherrypy.

Reviewer guidance

  1. Run ka-lite or some other program to occupy the port
  2. Run kolibri with the same port and see the output on terminal

References

#4189


Contributor Checklist

  • Contributor has fully tested the PR manually
  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If there are any front-end changes, before/after screenshots are included
  • If this is an important user-facing change, PR or related issue has a 'changelog' label

Reviewer Checklist

  • Automated test coverage is satisfactory
  • Reviewer has fully tested the PR manually
  • PR has been tested for accessibility regressions
  • External dependencies files were updated (yarn and pip)
  • Documentation is updated
  • Link to diff of internal dependency change is included
  • CHANGELOG.rst is updated for high-level changes
  • Contributor is in AUTHORS.rst

@lyw07 lyw07 added the TODO: needs review Waiting for review label Sep 26, 2018
@lyw07 lyw07 added this to the 0.11.0 milestone Sep 26, 2018
@codecov
Copy link

codecov bot commented Sep 26, 2018

Codecov Report

Merging #4333 into develop will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4333      +/-   ##
===========================================
- Coverage    52.92%   52.91%   -0.01%     
===========================================
  Files          718      718              
  Lines        22813    22810       -3     
  Branches      3096     3096              
===========================================
- Hits         12073    12070       -3     
  Misses       10017    10017              
  Partials       723      723
Impacted Files Coverage Δ
kolibri/utils/sanity_checks.py 100% <100%> (ø) ⬆️

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 25902c8...5cdf2bc. Read the comment docs.

@mrpau-richard
Copy link
Contributor

@lyw07 I tested this PR in Ubuntu, and it did display the port occupied errors in the terminal.
screen shot 2018-09-27 at 2 46 00 pm

But on Windows it's still saying at the Command prompt that the kolibri running on occupied port.(refer to the screenshot below)
screen shot 2018-09-27 at 3 28 13 pm

@lyw07 lyw07 force-pushed the port_not_available branch from a3ea584 to cc03798 Compare September 27, 2018 18:55
@lyw07
Copy link
Contributor Author

lyw07 commented Sep 27, 2018

@mrpau-richard thank you for testing! I just pushed the fix for Windows and tested the Windows installer. It should be working now :)

@mrpau-richard
Copy link
Contributor

Great work @lyw07 , I tested it at the Windows. The port error logs are now working in the Command prompt.
screen shot 2018-09-28 at 3 16 55 pm

Copy link
Contributor

@mrpau-richard mrpau-richard left a comment

Choose a reason for hiding this comment

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

I Tested this @lyw07.

@lyw07
Copy link
Contributor Author

lyw07 commented Sep 28, 2018

@mrpau-richard thank you so much for testing!

s.bind((host, port))
s.close()
except socket.error:
portend.free(host, port, timeout=2)

This comment was marked as spam.

Copy link
Contributor

@benjaoming benjaoming left a comment

Choose a reason for hiding this comment

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

Great + simplifying changes. portend is already a dependency of cherrypy, so no problem depending on it... just keep in mind that we'll follow the inferred cherrypy version requirements for this one.

@indirectlylit
Copy link
Contributor

made the timeout into a constant as requested

@indirectlylit indirectlylit merged commit 9337748 into learningequality:develop Oct 3, 2018
@indirectlylit indirectlylit removed the TODO: needs review Waiting for review label Oct 12, 2018
@lyw07 lyw07 deleted the port_not_available branch January 3, 2019 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants