-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Http server does not handle multiple requests correctly when pipeling disabled #29500
Comments
Pinging @elastic/es-core-infra |
Hi, can i fork and try playing with this issue??? |
When HTTP pipelining is disabled, a client that is sending a request before it has received the response for a previous request is misbehaving with respect to Elasticsearch (I am not saying they are misbehaving with respect to the HTTP/1.1 specification).
The default is for it to be enabled, and I agree the germane question is whether or not we should remove the ability for it to be disabled. From the HTTP/1.1 specification:
With this, I lean towards saying that we remove the ability to disable pipelining (i.e., pipelining is always enabled). |
Right. I worded it poorly, but removing the option for pipelining to be disabled is what I was suggesting.
I agree. Users could still set |
@tbrooks8 Let us move forward with removing the ability to disable pipelining. We can deprecate in 6.x. |
These tasks need to be completed:
|
This is related to #29500 and #28898. This commit removes the abilitiy to disable http pipelining. After this commit, any elasticsearch node will support pipelined requests from a client. Additionally, it extracts some of the http pipelining work to the server module. This extracted work is used to implement pipelining for the nio plugin.
This is related to elastic#29500. In 7.0 this setting will be removed. This PR marks the setting in 6.x as deprecated.
This is related to elastic#29500. We are removing the ability to disable http pipelining. This PR removes the references to disabling pipelining in the integration test case.
This is related to #29500. We are removing the ability to disable http pipelining. This PR removes the references to disabling pipelining in the integration test case.
This is related to #29500. In 7.0 this setting will be removed. This PR marks the setting in 6.x as deprecated.
Closing this as pipelining support is now mandator in 7.0 and the setting is deprecated in 6.x. |
It appears to me that we do not have protections to ensure that multiple http requests over a single connection are handled correctly when http pipelining is not enabled.
Essentially if a client sends multiple requests, we will receive those requests, parse them, and dispatch them to other threads for handling. Without the pipelining enabled, the first request to be handled will be the first response written back to the channel. This leads to out of order responses.
I'm not immediately sure what the fix is. We could always enable pipelining. Or maybe the server should send a something like a 501 if it receives a requests before the initial response has been returned?
The text was updated successfully, but these errors were encountered: