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

Add `with_timeout` API to Lua scripting #465

Merged
merged 1 commit into from Jul 1, 2016

Conversation

Projects
None yet
4 participants
@mike1808
Contributor

mike1808 commented Jun 7, 2016

No description provided.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 7, 2016

Current coverage is 86.05%

Merging #465 into master will increase coverage by 0.08%

  1. 2 files in splash were modified. more
    • Partials +2
    • Hits -2
  2. 3 files (not in diff) in splash were modified. more
    • Partials -5
    • Hits +5
@@             master       #465   diff @@
==========================================
  Files            40         40          
  Lines          4768       4812    +44   
  Methods           0          0          
  Messages          0          0          
  Branches        654        662     +8   
==========================================
+ Hits           4099       4141    +42   
- Misses          513        514     +1   
- Partials        156        157     +1   

Powered by Codecov. Last updated by 357608e...fa24aec

codecov-io commented Jun 7, 2016

Current coverage is 86.05%

Merging #465 into master will increase coverage by 0.08%

  1. 2 files in splash were modified. more
    • Partials +2
    • Hits -2
  2. 3 files (not in diff) in splash were modified. more
    • Partials -5
    • Hits +5
@@             master       #465   diff @@
==========================================
  Files            40         40          
  Lines          4768       4812    +44   
  Methods           0          0          
  Messages          0          0          
  Branches        654        662     +8   
==========================================
+ Hits           4099       4141    +42   
- Misses          513        514     +1   
- Partials        156        157     +1   

Powered by Codecov. Last updated by 357608e...fa24aec

@@ -66,6 +67,9 @@ def start(self, coro_func, coro_args=None):
self._waiting_for_result_id = self._START_CMD
self.dispatch(self._waiting_for_result_id)
def stop(self):
self._is_stopped = True

This comment has been minimized.

@kmike

kmike Jun 9, 2016

Member

looks good 👍

@kmike

kmike Jun 9, 2016

Member

looks good 👍

@@ -102,6 +106,9 @@ def dispatch(self, cmd_id, *args):
try:
args = args or None
if self._is_stopped:
raise StopIteration

This comment has been minimized.

@kmike

kmike Jun 9, 2016

Member

I'm not 100% sure this works correctly, or at least the code is not clear: the exception is caught later, and BaseScriptRunner tries to return the result by calling self.on_result(res), where res is lua2python(self.result). This looks unnecessary, and it is kind of hard to grasp what happens if on_result is called earlier, or if self.result is guaranteed to be None or if it can contain something else.

What do you think about raising another exception, and handling it explicitly? Or maybe just add a bit of comments to the source code which explain why does it work.

@kmike

kmike Jun 9, 2016

Member

I'm not 100% sure this works correctly, or at least the code is not clear: the exception is caught later, and BaseScriptRunner tries to return the result by calling self.on_result(res), where res is lua2python(self.result). This looks unnecessary, and it is kind of hard to grasp what happens if on_result is called earlier, or if self.result is guaranteed to be None or if it can contain something else.

What do you think about raising another exception, and handling it explicitly? Or maybe just add a bit of comments to the source code which explain why does it work.

This comment has been minimized.

@mike1808

mike1808 Jun 9, 2016

Contributor

self.result cannot be anything except an empty string, so calling it is harmless

@mike1808

mike1808 Jun 9, 2016

Contributor

self.result cannot be anything except an empty string, so calling it is harmless

Show outdated Hide outdated splash/qtrender_lua.py Outdated
Show outdated Hide outdated splash/qtrender_lua.py Outdated
Show outdated Hide outdated docs/scripting-ref.rst Outdated
Show outdated Hide outdated splash/qtrender_lua.py Outdated
Show outdated Hide outdated splash/qtrender_lua.py Outdated
Show outdated Hide outdated docs/scripting-ref.rst Outdated
@kmike

This comment has been minimized.

Show comment
Hide comment
@kmike

kmike Jun 9, 2016

Member

Overall the approach looks good, great work!
The PR needs tests to make sure edge cases are handled. The code turns out to be a bit tricky, so having good tests for it very much appreciated.

Member

kmike commented Jun 9, 2016

Overall the approach looks good, great work!
The PR needs tests to make sure edge cases are handled. The code turns out to be a bit tricky, so having good tests for it very much appreciated.

Show outdated Hide outdated docs/scripting-ref.rst Outdated
Show outdated Hide outdated splash/qtrender_lua.py Outdated
@kmike

This comment has been minimized.

Show comment
Hide comment
@kmike

kmike Jun 10, 2016

Member

The tests look good 👍, but some cases are missing:

Member

kmike commented Jun 10, 2016

The tests look good 👍, but some cases are missing:

@mike1808

This comment has been minimized.

Show comment
Hide comment
@mike1808

mike1808 Jun 10, 2016

Contributor

But I'm more concerned about if not qtimer.isActive(): return - when is it executed?

It happens when the timeout is elapsed but the callback didn't finished its job. In this case, StopIterration exception is raised and send_results is called wihn an empty string, after one of those callbacks is called. In that case qtimer.isActive() will return False.

Contributor

mike1808 commented Jun 10, 2016

But I'm more concerned about if not qtimer.isActive(): return - when is it executed?

It happens when the timeout is elapsed but the callback didn't finished its job. In this case, StopIterration exception is raised and send_results is called wihn an empty string, after one of those callbacks is called. In that case qtimer.isActive() will return False.

@kmike

This comment has been minimized.

Show comment
Hide comment
@kmike

kmike Jun 10, 2016

Member

@mike1808 aha, makes sense. Could you please add a test for this?

Member

kmike commented Jun 10, 2016

@mike1808 aha, makes sense. Could you please add a test for this?

Show outdated Hide outdated splash/lua_modules/wraputils.lua Outdated
Show outdated Hide outdated splash/qtrender_lua.py Outdated
Show outdated Hide outdated splash/lua_modules/wraputils.lua Outdated
Show outdated Hide outdated docs/scripting-ref.rst Outdated
Show outdated Hide outdated splash/qtrender_lua.py Outdated
@mike1808

This comment has been minimized.

Show comment
Hide comment
@mike1808

mike1808 Jun 30, 2016

Contributor

There is one "red" line in codecov-io that I cannot cover. I didn't find any case where this line can be executed.

Contributor

mike1808 commented Jun 30, 2016

There is one "red" line in codecov-io that I cannot cover. I didn't find any case where this line can be executed.

@immerrr

This comment has been minimized.

Show comment
Hide comment
@immerrr

immerrr Jun 30, 2016

Contributor

LGTM!

Contributor

immerrr commented Jun 30, 2016

LGTM!

@mike1808 mike1808 changed the title from [WIP] Add `with_timeout` API to Lua scripting to Add `with_timeout` API to Lua scripting Jun 30, 2016

@immerrr immerrr merged commit 88c3799 into scrapinghub:master Jul 1, 2016

2 checks passed

codecov/patch 95.65% of diff hit (target 90.00%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@immerrr

This comment has been minimized.

Show comment
Hide comment
@immerrr

immerrr Jul 1, 2016

Contributor

Merged! Thank you!

Contributor

immerrr commented Jul 1, 2016

Merged! Thank you!

@mike1808

This comment has been minimized.

Show comment
Hide comment
@mike1808

mike1808 Jul 1, 2016

Contributor

Nice, thank you!

Contributor

mike1808 commented Jul 1, 2016

Nice, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment