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
test/client_test.rb
Outdated
| 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
| @@ -234,8 +241,7 @@ def ensure_socket! | |||
|
|
|||
| 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
| @@ -59,7 +59,8 @@ def initialize( | |||
| json_serializer: nil, | |||
| custom_labels: nil, | |||
| 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