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

An ability to disable HTTP/1.1 KeepAlive support added #78

Merged
merged 3 commits into from Jan 26, 2018

Conversation

ssoftpro
Copy link
Contributor

No description provided.

There was a few problems with the loop to wait for thread in a pool to become ready and the most important - exit from Exeute method after the first successfull call to fThreadPoolPush inside the loop
This is the imlementation of changes discussed previously in https://synopse.info/forum/viewtopic.php?id=4334
Copy link
Owner

@synopse synopse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the logic change behind it... only difference seems that terminated will increase fThreadPoolContentionAbortCount counter...

SynCrtSock.pas Outdated
// HTTP/1.0 compatibility
// Only threads from thread pool will be used to process requests
// when this flag set to true
property DisableKeepAliveSupport: boolean read fDisableKeepAliveSupport;
Copy link
Owner

@synopse synopse Jan 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add "write" attribute too?
this may be enough, and no need to add a boolean parameter to the constructor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's don't forget that this is a thread so execution starts immediately. It could be too late to set this flag after execution started and first requests came
On the other side, IMHO it is non-practical to switch such behavior on the fly.

SynCrtSock.pas Outdated
@@ -1843,6 +1845,11 @@ THttpServer = class(THttpServerGeneric)
// a THttpServerResp thread is created for handling this THttpServerSocket
property Sock: TCrtSocket read fSock;
published
/// flag to disable HTTP/1.1 keep alive features and fall back to
// HTTP/1.0 compatibility
// Only threads from thread pool will be used to process requests
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing traiiling "-" character in comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change committed. Did I understand you correctly?

@ssoftpro
Copy link
Contributor Author

ssoftpro commented Jan 26, 2018

The primary point of change in the loop is to change this part:
if fThreadPoolPush(pointer(ClientSock)) then
exit; // the thread pool acquired the client sock
'exit' here results in stop execution of the Execute method in whole, so server stops it's execution at all
The correct behavior should be to stop execution of internal 'for' loop and continue execution of external 'while' loop and not increase fThreadPoolContentionAbortCount when fThreadPoolPush returned true
I would appreciate if you have a better solution for this.

@synopse synopse merged commit 29751d0 into synopse:master Jan 26, 2018
@synopse
Copy link
Owner

synopse commented Jan 26, 2018

now I understood the problem of the threadpool contention loop !
this was indeed a nasty bug

@ssoftpro ssoftpro deleted the fb/THttpServer_DisableKeepAlive branch January 26, 2018 17:12
@synopse
Copy link
Owner

synopse commented Jan 26, 2018

I've made some changes to how the contention is handled - especially changed ThreadPoolContentionCount into ThreadPoolContentionTime and enhanced the doc

@synopse
Copy link
Owner

synopse commented Jan 26, 2018

the commit broke the TWebSocketServer.Create signature, by the way

  • I decided to change the boolean parameter into KeepAliveTimeOut, with 0 = disable

@synopse
Copy link
Owner

synopse commented Jan 26, 2018

the commit also broke THttpServerSocket.GetRequest with no associated server
(it made a GPF and failed TTestProtocols.RTSPOverHTTP)
-> please ensure in the future that you run TestSQL3 regression tests before any pull request ;)

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

Successfully merging this pull request may close these issues.

None yet

2 participants