Seldaek
commented
3 years ago I guess ideally we'd open a single curl handle, use http/2, and pipe all requests through that without closing it until close or perhaps reset happens.
Especially with a curl_multi-style implementation this would allow it to run the requests in the background, without us having to explicitly fetching the responses, although I guess we could still fetch the first few bytes of the response before closing to ensure delivery.
This would be a larger undertaking though, especially to port all handlers to this, and make sure we have a reusable API for it which works well and is covering all needed features.
Symptom
About half of the messages are dropped in an undeterministic fashion. This applies to single messages as well as bulk-sending e.g. 20 messages.
How to reproduce
I put together a test case at cbcf/monolog-pushover-handler-bug-demo. This also contains rudimentary performance measures for the different approaches listed below.
Probable cause and fixes
The base SocketHandler takes a shortcut by only writing to the socket. Since it never reads back the response(s), most servers are likely to stop accepting new requests since response buffer is filling up. Prematurely closing the connection may causes requests to be dropped server-side before reaching the actual processing stage. This is especially true if there is an intermediate server. Currently, for the Pushover API, the response headers indicate that cloudflare is used.
Closing the socket connection after every request was introduced in PR #148 (discussion #146).
Reading back a bit of response data seemed to help with SlackHandler #729 and HipChatHandler #1116. I tested this in PushoverHandlerWithFread.php. This seems to fix the primary symptom, but it is a significant slowdown for bulk-sending.
On the other hand, pushover recommend keeping the connection open for successive messages. I put together a proof-of-concept for reading the HTTP response bodies in PushoverHandlerWithKeepAlive.php. For my machines, this was only slightly slower than the current implementation and much faster than closing the socked for every write.
Conclusion/Opinion
Pushover it is targeted for medium-volume notifications (7500 messages per month). Therefore, the actual performance penalty of the "clean" solution may not justify implementing a full response read.
In my view, it would be well justified to actually interpret the response to ensure the messages was actually sent and accepted by the API. Throwing a proper exception would allow for fallback handlers. Due to the montly limit, I think Pushover is mostly suited for rare, high-priority notifications which demand such fallbacks.
As I understand, curl and the like were intentionally not used. Do you think adding a kind of "custom" HTTP response reader as an optional functionality of SocketHandler is desirable (in a separate feature issue I guess)?