AstarVienna / skycalc_ipy

iPython version of skycalc_cli
GNU General Public License v3.0
4 stars 7 forks source link

Use https and add request timeout #7

Closed joao-aveiro closed 1 year ago

joao-aveiro commented 1 year ago

Change URLs to https instead of https to enable encryption. If it really is required that http (without SSL/TL) can be used, then some additional logic must be added, but, even so, I believe httpsshould be the default.

Additionally, include timeouts so that requests don't hang eternally. This is useful when the request is not successsful but an exception is not raised (per chance, packets are dropped but the request doesn't fail completely?). I am using 1s as a default, which worked for my case, but be free to change it. Note that this timeout is not the time taken to complete the request, but for how long it is allowed to wait while the server is unresponsive.

Fixes #8 and fixes #9.

joao-aveiro commented 1 year ago

@astronomyk @tomasstolker @oczoske My apologies for tagging everyone in the contributors list, but have you had a chance to look into this PR? I believe it is straightforward enough and it would fix the problems we are experiencing in our campus where we can't use ScopeSim (which uses this package) because of this issue.

hugobuddel commented 1 year ago

Sorry, I should have noticed this! 1 second seems a bit short, but I suppose you have determined it is long enough, so I'll merge this.

hugobuddel commented 1 year ago

Aj, as I was ready to release new version of skycalc_ipy (and other ScopeSim projects) I noticed that skycalc_ipy can do a sys.exit() when the request times out! Exiting might be acceptable for a command line tool that skycalc_ipy is as well, but not for a library. The library should just raise the Exception again.

Seems easy enough to change, by modifying stop_on_errors_and_exceptions. However, setting that to False seems to just ignore the exception, which might not be any better. And now we get a tri-state Boolean, 'ignore', 'exit', 'raise'... I think I'm just going to change this, because the stop_on_errors_and_exceptions is not set anywhere in any of our projects.

I'll also increase the timeout to 2 seconds or so. The CI now runs 15 tests (python 3.8, 3.9, 3.10, 3.11 x linux, mac, windows + 3 more), and it happens often that one of those fail.

joao-aveiro commented 1 year ago

I am not sure I'm understanding the problem correctly and I must admit I haven't looked much into the exception-handling logic. However, the exit must occur at L386 at core.py. I don't believe that those lines should be there. More so, I don't know if I understand what you are trying to achieve with handle_error() and handle_exception(). Some thoughts:

  1. First of all, these methods seem somewhat redundant, and if there is a "custom error" occurring, a custom exception should be defined and it should be raised instead and handled with an except clause (and further with your custom handler if you wish, but only inside the except body). Thus, only handle_exception() should need to exist.
  2. These methods don't seem to actually handle much; the messages are printed, yes, but the exceptions are kept unhandled - as you mentioned - unless an exit code is triggered. At first glance, I believe self.stop_on_errors_and_exceptions should be false for normal API use and, in that case, the exception caught should be re-raised if no further handling/recovery can be done - which is probably the case for most of these exceptions.
  3. The connection timeout is indeed not being handled correctly; it should be done so by importing the respective exception, from requests.exceptions import ConnectTimeout, and adding a new except block for this. Then, if the previous suggestions are implemented, a ConnectTimeout exception will be handled and ultimately re-raised, with no exit signal being created.
  4. I am supposing that self.stop_on_errors_and_exceptions can be True and, in that case, an exit signal will arise which is the plan. I am obviously overlooking how this self.stop_on_errors_and_exceptions attribute may be set automatically depending on whether the package is being imported/used programmatically or it is being used as a standalone/interactive/etc. program. There must be ways to achieve this, but I haven't dwelled on it. Nevertheless, should this even happen? In either case, shouldn't you just let the code raise exceptions and eventually exit naturally if it leads to that? I am genuinely asking since I don't know what you are trying to achieve. At first glance, I would think that there shouldn't be any sys.exit() here and only normal exceptions being raised.
  5. Finally, I would believe that this sys.exit() that you see during the tests would not only be triggered by the connection timing-out, but also by anything that might set deleter_response in L386 to something other than 'ok'. Perhaps the tests didn't trigger this before (?).

P.S.: Now re-reading your comment, I believe you are suggesting something similar with your "tri-state boolean" (which I admit I didn't understand that sentence at first). However, I wouldn't define states/enums/...; instead, I woould simply re-raise the exception by default and optionally (or perhaps never?) do a sys.exit(). But once again I don't exactly know your reasoning and your use cases, so I might be missing something.