docker / cli

The Docker CLI
Apache License 2.0
4.74k stars 1.88k forks source link

plugins: cleanup sockets when done #5146

Closed laurazard closed 3 weeks ago

laurazard commented 3 weeks ago

- What I did

Since 509123f935e6fcb2cedf0df7bd8585a346082969, we've been leaking sockets in the filesystem on platforms where abstract sockets aren't supported.

That change relied on Go to cleanup our sockets for us, which Go will happily do as long as we make sure to close the listener, which we weren't previously doing unless to signal the plugin to terminate.

- How I did it

This change adds a deferred call to PluginServer.Close(), which makes sure we close the plugin server at the end of the plugin execution, so that we never exit without cleaning up.

- How to verify it

Execute a plugin command with debug logging enabled (e.g. docker -D compose convert) and look for the added debug log. Also check $TMPDIR for leaked sockets.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Screenshot 2024-06-11 at 21 32 13

codecov-commenter commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Project coverage is 61.67%. Comparing base (591bd17) to head (3dcc653). Report is 14 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #5146 +/- ## ========================================== - Coverage 61.82% 61.67% -0.16% ========================================== Files 298 295 -3 Lines 20730 20778 +48 ========================================== - Hits 12817 12815 -2 - Misses 7000 7043 +43 - Partials 913 920 +7 ```
thaJeztah commented 3 weeks ago

@laurazard how difficult would it be to write a test-case for this? (could be in a follow-up if we want to get this in)

thaJeztah commented 3 weeks ago

Had a quick chat; let's bring this one in, and do a follow-up for the test and any other changes we may need.