search

Consensys / armlet

a MythX API client wrapper
MIT License
17 stars 7 forks source link

Remove the default timeout #42

Closed muellerberndt closed 5 years ago

muellerberndt commented 5 years ago

At the moment (in Armlet 1.1.0) the default request timeout is set to 30 seconds, with is too short for a quick analysis to complete. Also, this assumes by default that the user isn't running a full analysis.

I suggest that we do not set an timeout in armlet by default as this is handled on the server side (we can still support it as an optional parameter. WDYT?

rocky commented 5 years ago

Funny I was looking at this today See https://github.com/ConsenSys/armlet/pull/43

The way the timeout in armlet works is pretty weird in armlet because it doesn't really stop the backend from continuing. So the next time you run an analysis with the same timeout it might succeed immediately as it has finished the first time and is now cached.

To be clear, there are two concepts: how long you want the back-end analysis to take before the back end will give up, and how long the user wants to wait before getting a response whether or not the back end continues.

In an IDE, this first concept isn't very important, because other things are going on in another process on another computer and the response can come asynchronously. The main place where the back end running for an unbounded amount of time can be an annoyance is in the truffle analyze cli (or something like that), which is perhaps the biggest use of armlet. Even in a truffle develop console, truffle doesn't allow for asynchronous events in its REPL (yet). So there may be a nicety for having the two kinds of timeouts separated.

That said, from a practical standpoint I don't see this as having any significant impact one way or another.

muellerberndt commented 5 years ago

The way the timeout in armlet works is pretty weird in armlet because it doesn't really stop the backend from continuing.

Exactly, it's only useful as a "last resort" if the analysis is definitely stuck in some way. For example a quick analysis is expected to finish within at most 60-90 seconds, but should never take more than 5 minutes. After 5 minutes, the client can safely assume that something is wrong and drop the connection.

rocky commented 5 years ago

Exactly, it's only useful as a "last resort"

That is one case, but not the only one.

The client side understands the context better of what the end-user is doing.

The context may be a CLI where the user has to wait before anything is done, e.g. truffle-anayze cli, and thus we want to allow the user a more granular way to indicate how impatiient he/she is in addition to "quick"/"full" which feels to me more about quality of analysis, not time.

Or the context can be an IDE where asynchronous reporting of results is possible. But even here the IDE may decide to set a limit very small, say at 3 seconds, for more extreme interactive-like behavior. If it gets something, great - it will report it. But If not, because MythX hasn't had enough time, that's okay because it will issue another request as soon as the first returns. Since the likelihood of things not changing in such a small interval is pretty great, on the next request, we can take advantage of the fact that the backend cached the first result if it completed.

And as you suggest above, there probably should always be some "last-resort" time to wait before giving up because something really went bonkers. But isn't that essentially a "default value"?

muellerberndt commented 5 years ago

as you suggest above, there probably should always be some "last-resort" time to wait before giving up because something really went bonkers. But isn't that essentially a "default value"?

@rocky yes, but like you said, the client side understands the context better of what the end-user is doing. Armlet shouldn't be forcing any particular timeout on the client, so I'd argue that there should be no timeout at all by default. If the client wants one, they can set it explicitly.

birdofpreyru commented 5 years ago

+1 for no unlimited default timeout. The backend guarantees (assuming there is no bugs) that eventually any analysis request will end up either with Finished (on success) or Error (on failure) status. Thus the only practical purpose of timeout now is to limit the maximum number of poll pings the Armlet can send (as they are rate limited). Going forward, once we add the endpoint to remove a non-started analysis from API queue, we can also add an optional call to that in the end of timeout. So that client can express that after that time I won't need results of that analyses anyway, thus please drop it, if not started yet, saving some API usage and queue time for my subsequent requests.

muellerberndt commented 5 years ago

Merged #60

rocky commented 5 years ago

Like @birdofpreyru I agree not having a default timeout is bad. If you want to set it to a high number like 24 hours, that's fine.

And in #60 I misunderstood in my sleep deprevation that this was going to be applied to the default value, not an example value. That too feels wrong. If the example goes with a "quick" analysis, as it does here, it should be set to something slightly larger than the quick analysis default of 90 seconds, say 120 seconds. Not one day.

I've written two clients that use armlet. In neither do I feel that the right thing to do is use 1 day.

I often run the example analysis program, and when I run that as a demo, I also feel 1 day is the wrong value too.

So given a sample size of 2 clients, and my own use what is currently in the example is just something that feels to me not right. If people cut and paste this and don't change the value, they will be starting out with something not right.

So, if you would, I would appreciate it if you would clarify why

rocky commented 5 years ago

Also, and this is important, it really messes up our ability to do geometric polling in a way that, by design, favors quick response for quick jobs over jobs which are long.

Therefore as currently written when geometric delay is in place, the example analysis program will demonstate how to get poor reponse via longer polling values on quick analysis jobs, which is something I don't think we want to show off.