Making Transport class thread safe and fix leaking connection#132
Making Transport class thread safe and fix leaking connection#132alxtkr77 wants to merge 3 commits intov3io:developmentfrom
Conversation
| with self._lock: | ||
| if self._closed: | ||
| raise RuntimeError("Transport closed during response handling") from e | ||
| connection = self._create_connection(self._host, self._ssl_context) |
There was a problem hiding this comment.
Doesn't seem like this line belongs in the locked section.
There was a problem hiding this comment.
It can be and it does not affect the performance. Eventually it calls http.client.HTTPConnection, which should be thread safe, but since all it does is initializing some structures without actually doing any connecting, I prefer to have it under the lock.
There was a problem hiding this comment.
But why should it be under the lock if it doesn't require locking?
|
|
||
| starting_offset = request.body.tell() if is_body_seekable else 0 | ||
| retries_left = self._request_max_retries | ||
| current_connection = connection # Track the current connection for thread safety |
There was a problem hiding this comment.
Maybe I'm missing something, but aliasing connection as current_connection doesn't seem to have any effect in this function.
There was a problem hiding this comment.
The aliasing of current_connection = connection is useful because it creates a mutable reference that can be updated during retry attempts while preserving the original parameter. This allows you to track which connection is active at any point in the retry loop, especially when you need to create new connections after failures.
There was a problem hiding this comment.
But the original connection variable is never used after aliasing.
| with self._lock: | ||
| if self._closed: | ||
| raise RuntimeError("Transport closed during request retry") from e | ||
| current_connection = self._create_connection(self._host, self._ssl_context) |
There was a problem hiding this comment.
Here too, it doesn't seem like creating the connection belongs in a locked section.
| with self._lock: | ||
| if self._closed: |
There was a problem hiding this comment.
Consider using double-checked locking, especially here since it's on the hot path.
There was a problem hiding this comment.
Since all the locked sections are not performing any time consuming operations - even on the fast path - I don't see a reason to use double-checked locking
There was a problem hiding this comment.
Double-checked locking is always only to avoid locking unnecessarily, unrelated to the content of the block.
| # Return connection to pool even on HTTP errors, as the connection is still valid | ||
| with self._lock: | ||
| if not self._closed: | ||
| try: | ||
| self._free_connections.put(connection, block=False) | ||
| except Exception as e: | ||
| self._logger.warn_with( | ||
| "Failed to return connection to pool", exception=str(e), connection_id=id(connection) | ||
| ) | ||
| connection.close() |
There was a problem hiding this comment.
As discussed, this change would introduce a connection leak rather than fixing one.
|
Replaced by #133. |
2 participants
https://iguazio.atlassian.net/browse/ML-9429