LDEV-2332 configurable request timeout behaviour

[DominicWatson]

See https://luceeserver.atlassian.net/browse/LDEV-2332.

This pull request allows the setting of two environment variables to control request timeout behaviour in Lucee:

1. lucee.enable.request.timeout: whether or not request timeouts are handled at all
2. lucee.enable.request.timeout.monitor: whether or not request timeouts are monitored by the controler thread (lucee.enable.request.timeout must be true as well)

Both are true by default so that no behaviour changes without setting either of these variables to false.

Adds "in thread" timeout checking

In addition to checking whether or not timeouts are enabled, the PR also adds a number of checks in various places in code execution for whether or not the current request thread has run into a timeout. These checks are designed as a basic backstop for when lucee.enable.request.timeout=true (default) and lucee.enable.request.timeout.monitor=false (non-default).

It would be interesting to see if there are more places where this check could sensibly be introduced, i.e. at the start of each loop iteration, etc. I have started with a few examples. But there may also be a completely different, but workable approach to allowing same thread timeout checking.

---

NB: diff best viewed with whitespace changes suppressed. Have my editor configured to trim trailing whitespace.

[michaeloffner]

this pull request is for the master branch, we do never coding on the master branch directly, the master branch only see merges of branches that get releases. please make a pull request to 5.3 or 6.0

[michaeloffner]

this PR affects the loader what would make necessary for EVERY user to manually update the lucee.jar, we only make this for MAJOR updates. Lucee 4 to 5 or Lucee 5 to 6. BUT for Lucee 6 no loader update is planned, means the next loader change will happen with Lucee 7 or later. Also every loader update makes it necessary to raise the loader version and update the requirement definition for the core. In short you need to remove all loader changes.

[michaeloffner]

this (and similar) allowRequestTimeout( Caster.toBoolean(SystemUtil.getSystemPropOrEnvVar("lucee.enable.request.timeout", null) , true) ); need to be changed to this allowRequestTimeout( Caster.toBooleanValue(SystemUtil.getSystemPropOrEnvVar("lucee.enable.request.timeout", null) , true) ); because it makes 2 unnecessary Autoboxing/Unboxing from Boolean to boolean and back

[michaeloffner]

i did not check in detail yet, do you make sure that gateway threads still are NEVER running in a request timeout. this is very important.

[michaeloffner]

i try to keep the amount of methods as low as possible, the following methods seem not necessary getRequestHasTimedOut,setRequestHasTimedOut you can use getTimeoutStackTrace()!=null instead.

[DominicWatson]

i try to keep the amount of methods as low as possible, the following methods seem not necessary getRequestHasTimedOut,setRequestHasTimedOut you can use getTimeoutStackTrace()!=null instead.

Does adding the methods cause performance issue? getRequestHasTimedOut() tells the developer reading the code exactly what the code means, whereas they would need to guess what getTimeoutStackTrace()!=null means. Also - you then can't change the implementation of the timeout + the timeout stack trace without effecting code that relies on checking this.

This seems counter intuitive to me. Perhaps, more sensible may be to drop the setRequestHasTimedOut() and have getRequestHasTimedOut() just return getTimeoutStackTrace()!=null; rather than repeat that internal logic in multiple places?

[DominicWatson]

i did not check in detail yet, do you make sure that gateway threads still are NEVER running in a request timeout. this is very important.

No - the in-request timeout checking definitely needs checking. That said, if that code is running from within the thread - what is the risk of throwing a timeout exception without checking gateway threads?

[DominicWatson]

this PR affects the loader

Can you point out which changes effect the loader so I might be able to mitigate? Or you think its not possible to do so.

I think it makes sense to overhaul the timeouts as part of Lucee 6. Here, I am really looking for a way that we can choose to disable them without any compatibility breaking.

Perhaps if we can re-focus this PR on just disabling the timeout monitor altogether, that could work.

[DominicWatson]

this pull request is for the master branch, we do never coding on the master branch directly, the master branch only see merges of branches that get releases. please make a pull request to 5.3 or 6.0

Ok, will close and start a new one.

I think it would be good to have a separate discussion around the release process / contribution guidelines though in this case. There is no way for a developer to know right now.

[zspitzer]

Would an application.onRequestTimeout method help here?

[DominicWatson]

Would an application.onRequestTimeout method help here?

It won't help the situation in anyway, but would be an interesting feature. As Micha points out, it's only ever going to be fired when a request timeout is reached that is not caused by some process that runs for ever. i.e. its only going to be fired on a general slow down that eventually means you bump into a timeout check in the request.

But for me, that is slightly better than nothing - and a whole lot better than killing threads.

[zspitzer]

add possibility to add a cpu/memory/concurrent request threshold for request timeout

https://luceeserver.atlassian.net/browse/LDEV-3019

[michaeloffner]

@cfmitrah the underlaying ticket is already set to resolved, if that PR still resolves another issue or improves the current fix, please create a new ticket that links the current and create a new PR based on this one.