Fix async calls #64
Fix async calls #64
Conversation
This plugin could sometimes deadlock when the downstream server responded quickly. This patch fixes this by making sure the callbacks are declared before the request is sent. Fixes elastic/logstash#7588 (comment)
CHANGELOG.md
Outdated
| @@ -1,3 +1,6 @@ | |||
| ## 4.3.1 | |||
| - Fix deadlock that could occur in certain odd situations. If you noticed this plugin stalling this should fix it. | |||
There was a problem hiding this comment.
can we describe this a bit more or just say it fixes a deadlock? it seems the deadlock would occur pretty much every time in any situation?
There was a problem hiding this comment.
@jsvd well, I actually couldn't repo it against a slow endpoint. I think from a user's perspective this is in the category of must upgrade. I think it makes sense to modify the message to say "All users should upgrade, even if they aren't experiencing problems".
I don't know if the changelog is the place to go deep into code however.
lib/logstash/outputs/http.rb
Outdated
| end | ||
| end | ||
|
|
||
| request.call # Actually invoke the request in the background |
There was a problem hiding this comment.
is it worth describing a bit more why this fixes the issue? I'm ok with putting that explanation in the commit, but here would prevent future errors. if someone decides to "refactor" the order of these things
There was a problem hiding this comment.
@jsvd Sure, I'm glad to put that message in here
|
Testing this against nginx, which would see this request every ~ 50 requests, I now find that things work: The extra 1 line is accounted for by some whitespace I made when clearing the log file with |
There was a problem hiding this comment.
LGTM, please squash, merge and publish. Great job hunting this down, @andrewvc and @original-brownbear <3
Fixes elastic/logstash#7588 (comment) by correctly ordering calls to manticore.
Also removes a dead function that used to be the thing that sent requests.