FIX: Ensure socket is closed when error is raised while opening socket #328
Conversation
tgxworld force-pushed the ensure_socket_close branch 2 times, most recently from 8141df9 to 6f79322 Compare 
| def test_close_socket_on_error | ||
| logs = StringIO.new | ||
| logger = Logger.new(logs) | ||
| logger.level = :error | ||
| | ||
| client = PrometheusExporter::Client.new(logger: logger, port: 321, once: true) | ||
| client.send("put a message in the queue") | ||
| | ||
| assert_includes( | ||
| logs.string, | ||
| "Prometheus Exporter, failed to send message Connection refused - connect(2) for \"localhost\" port 321", | ||
| ) | ||
| end |
There was a problem hiding this comment.
This test does not directly test that the socket is closed but it does cause the internal close_socket! method to be called.
There was a problem hiding this comment.
Why aren't we explicitly checking for socket closure though?
There was a problem hiding this comment.
Because it is an internal implementation detail and there is basically no way for us to assert for that in the test since the reference to the socket is thrown away after it is closed.
tgxworld force-pushed the ensure_socket_close branch from 6f79322 to dbd8288 Compare | | ||
| nil | ||
| rescue StandardError | ||
| @socket = nil |
There was a problem hiding this comment.
Previously, we were just assigning the @socket to nil which causes a TCPSocket to remain open.
There was a problem hiding this comment.
Do we need an explicit ensure close_socket! in worker_loop?
There was a problem hiding this comment.
No the worker loop is meant to ensure that a socket is opened and reuse the socket across loops. Is there a reason you think we should close the socket each time we process a message?
There was a problem hiding this comment.
Not so much for each time we process a message, but I'm tracing the flow right now and I do see other places e.g. process_queue, where there's @socket = nil as well
prometheus_exporter/lib/prometheus_exporter/client.rb
Lines 146 to 147 in cf7bf84
There was a problem hiding this comment.
Good catch and I've made the change to call close_socket!. However, I don't think we can easily test for this and I think it is OK since the risk of dropping a message is low.
lib/prometheus_exporter/client.rb Outdated
| logger: Logger.new(STDERR), | ||
| log_level: Logger::WARN | ||
| log_level: Logger::WARN, | ||
| once: false |
There was a problem hiding this comment.
Sorry can you elaborate here what is "once: false"?
Export it once only and kill after? Perhaps some verbosity in variable naming can help
There was a problem hiding this comment.
I updated to process_queue_once_and_stop which should be much clearer.
tgxworld force-pushed the ensure_socket_close branch from dbd8288 to 4f308f2 Compare 
tgxworld force-pushed the ensure_socket_close branch from 4f308f2 to a5f8af5 Compare 
| Thank you for reviewing @nattsw 👍 |
| 2.2.0 - 2024-12-05 | ||
| | ||
| - FIX: Ensure socket is closed when error is raised while opening socket | ||
| - Feature: Add Dalli::Client memcache metrics for web_collector |
There was a problem hiding this comment.
Nitpick:
- FEATURE: Add Dalli::Client memcache metrics for web_collector 
/139110