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

fixed httpserver.r2py to work properly for Repy v2 and new unit test for it #150

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

us341
Copy link

@us341 us341 commented Oct 9, 2014

No description provided.

@us341 us341 changed the title update httpserver.r2py fixed httpserver.r2py to work properly for Repy v2 and new unit test for it Oct 22, 2014
@choksi81
Copy link
Contributor

  • I ran the unit-test designed to test the updated httpserver.r2py and the test got pass. So no issues there.
  • There is one thing I would like to mention though. There is a call to httpserver_sendfile_chunked in httpserver.r2py (https://github.com/SeattleTestbed/seattlelib_v2/pull/150/files#diff-d1237daea38f8ad8d8d5a35da2e295b2R895) which in turn calls filelikeobj.read(). This "read" call or the call to " httpserver_sendfile_chunked" hasn't been handled under try-catch block to catch the exceptions. I may be wrong, but this is one issue I though I would mention.
  • Otherwise the PR is good to be merged.

# raises. However, it just raises plain exceptions.
# Read chunks from the callback and efficiently send them to
# the client using HTTP/1.1 chunked encoding.
_httpserver_sendfile_chunked(sock, messagestream)
Copy link
Contributor

Choose a reason for hiding this comment

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

May be try-catch block is required here or in httpserver_sendfile_chunked to handle the "filelikeobj.read" exceptions?

Copy link
Author

Choose a reason for hiding this comment

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

filelikeobj is object of _httpserver_StringIO class and read() is calling this class's read method. It is handling the ValueError if the class object is closed.

httpretrieve.r2py is left intentionaly as dylink.r2py needs some ammendment
restrictions.threeports is added in place of restrictions.default as two different ports were needed for callback method hosted on server and client
Also mentioned the reason in comments to leave httpretrieve.r2py module with dy_import_module_symbols
Changed waitforconn docstring for callback function to take five arguments instead of one.
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.

2 participants