Skip to content

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.

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