Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Negative delay when using Sqlite throttle middleware #48

Closed
BeneCollyridam opened this issue Apr 29, 2021 · 2 comments · Fixed by #49
Closed

Negative delay when using Sqlite throttle middleware #48

BeneCollyridam opened this issue Apr 29, 2021 · 2 comments · Fixed by #49

Comments

@BeneCollyridam
Copy link
Contributor

BeneCollyridam commented Apr 29, 2021

When using the library for an extended time we eventually got the following error:

  • Your operating system name and version.
    • We got the error in Docker (Linux d65ebfed45f6 4.19.121-linuxkit x86_64 GNU/Linux) hosted on macOS Bug Sur (x84_64)
  • Any details about your local setup that might be helpful in troubleshooting.
    • It is a very long running program, so it seems like a bug that only happens in rare cases
  • Detailed steps to reproduce the bug.
    • Keep sending queries for ~20 hrs. It happens reliably for us but very rarely
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
...
~/.heroku/python/lib/python3.9/site-packages/epo_ops/api.py in published_data_search(self, cql, range_begin, range_end, constituents)
     90     ):
     91         range = dict(key="X-OPS-Range", begin=range_begin, end=range_end)
---> 92         return self._search_request(
     93             dict(
     94                 service=self.__published_data_search_path__, constituents=constituents
~/.heroku/python/lib/python3.9/site-packages/epo_ops/api.py in _search_request(self, info, cql, range)
    220     def _search_request(self, info, cql, range):
    221         url = self._make_request_url(info)
--> 222         return self._make_request(
    223             url, {"q": cql}, {range["key"]: "{begin}-{end}".format(**range)}
    224         )
~/.heroku/python/lib/python3.9/site-packages/epo_ops/api.py in _make_request(self, url, data, extra_headers, params, use_get)
    170             request_method = self.request.get
    171 
--> 172         response = request_method(url, data=data, headers=headers, params=params)
    173         response = self._check_for_expired_token(response)
    174         response = self._check_for_exceeded_quota(response)
~/.heroku/python/lib/python3.9/site-packages/epo_ops/models.py in post(self, url, data, **kwargs)
     84 
     85     def post(self, url, data=None, **kwargs):
---> 86         return self._request(_post_callback, url, data, **kwargs)
     87 
     88     def get(self, url, data=None, **kwargs):
~/.heroku/python/lib/python3.9/site-packages/epo_ops/models.py in _request(self, callback, url, data, **kwargs)
     93 
     94         for mw in self.middlewares:
---> 95             url, data, kwargs = mw.process_request(self.env, url, data, **kwargs)
     96 
     97         # Either get response from cache environment or request from upstream
~/.heroku/python/lib/python3.9/site-packages/epo_ops/middlewares/throttle/throttler.py in process_request(self, env, url, data, **kwargs)
     18         if not env["from-cache"]:
     19             service = service_for_url(url)
---> 20             time.sleep(self.history.delay_for(service))
     21         return url, data, kwargs
     22 
ValueError: sleep length must be non-negative

When I dig into the code it seems that it is in the calculation here epo_ops/middlewares/throttle/storages/sqlite.py#L123

I think it happens if the next run is in the past (aka. next_run < _now). It could probably be fixed by adding a check for that and returning 0

I have made a PR that should fix it, but maybe I'm missing something/not doing it the desired way: #49

@gsong
Copy link
Member

gsong commented Jul 20, 2021

@BeneCollyridam Have you been able to try your fix to see if it resolves the issue? I don't have a way of testing something that long-running myself currently.

@BeneCollyridam
Copy link
Contributor Author

@gsong We don't get the error anymore, so it is working for us. We've deployed it and have been using it since I made the MR.

@gsong gsong closed this as completed in #49 Sep 19, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants