Skip to content
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

Allow setting maximum execution time for builtins #361

Open
przemyslaw-dobrowolski-cl opened this issue Jan 26, 2017 · 5 comments
Open

Allow setting maximum execution time for builtins #361

przemyslaw-dobrowolski-cl opened this issue Jan 26, 2017 · 5 comments

Comments

@przemyslaw-dobrowolski-cl
Copy link
Contributor

If an extension is called that uses builtins like gohan_sync_watch, the time spent in the builtin is included in the total time spent in the extension. Since, the default time limit for an extension execution is 30 secs, it effectively disallows long-polling from extensions with an arbitrary timeout. In large scale solutions, 30 secs long-polling is not enough.

A proposed change is to add a configuration option 'builtin-timelimit' and to not include time spent in a builtin to the total execution time of the executing extension. A time spent in a builtin should counted separately and be limited according to the configuration setting mentioned above.

@nati
Copy link
Contributor

nati commented Jan 27, 2017

I would rather prefer set timeout for each extension.

@marcin-ptaszynski
Copy link
Contributor

marcin-ptaszynski commented Jan 27, 2017

@przemyslaw-dobrowolski-cl , IMO overly complicated method without clear goal. If we want to support long poll built on gohan_sync_watch, what does it matter, how long we spend in js code and builtin code? I think developer is more interested in total value anyway.

@nati , do you suggest using per (path template, method) tuple override? Or do you want to extend extension schema to provide optional timeout property (which would effectively set different timeout per loaded file)? This may be more difficult, since all extensions are executed from js.

Another idea is to place override builtin in extension, although it may be difficult to manage later.

@nati
Copy link
Contributor

nati commented Jan 30, 2017

@marcin-ptaszynski my suggestion is to extend extension schema to have own timeout.
I believe this is simple to implement since we can override default value by this new value when we run extension code.

@przemyslaw-dobrowolski-cl
Copy link
Contributor Author

I'm not sure if in the proposed implementation it is possible to determine the maximum running time for a particular extension in HandleEvent() / otto.go. Assuming that there is a declaration in schema/extensions section of the maximum running time for all extensions declared in this schema, one would need to track in Environment a mapping from an event type and a list of registered event receivers to the maximum running time. Am I missing something?

@nati
Copy link
Contributor

nati commented Feb 1, 2017

@marcin-ptaszynski @przemyslaw-dobrowolski-cl
I got what you meant after rechecking code.

This is the code we implement timeout.
https://github.com/cloudwan/gohan/blob/master/extension/otto/otto.go#L184

so I believe it is trivial change if we set time per path & event.

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

No branches or pull requests

3 participants