Enable unix sockets for Client creations:

[Kropyls]

Fixes: #325

Hey everyone, I saw that this had been sitting in limbo for a while with another PR that seems dead, and this would be a nice-to-have for me in my day-job, so I tried my own take at it. I'm always open to all negative/positive feedback.

What do these changes do?

This enables users to create a Client() with the "path" keyword for users who need a local socket connection. The general idea was to try to do this in a way that didn't break any per-existing consumers of the code, so I forced the path into a keyword argument so all existing behavior goes unaffected.

Details below:

client.py:

• Flag client now uses a port value of 0 to infer a unix socket
• holds onto a self._unix boolean (mostly for easy referencing)
• Client takes overloads for host/port or path combo
• if path is provided, overwrites any host/port value given

pool.py:

• change self._host to self._target to make clear that it could be IP or path
• add self._unix and self._opener values
• add _make_opener to MemcachePool:
  • this controls generating an opener function that will be slightly different for domain/ip sockets
• _create_new_conn now calls the self._opener value to open connections

simple.py:

• add a comment explaining how to use local sockets, and showing that it won't break existing code

commands_test.py:

• add a test for domain socket setting/setting

conftest.py:

• build up Params around unix sockets following the existing pattern for IP-based socks
• fix host assignment in mcache_server_docker

pool_test.py:

• add test for unix socket aquire/release
• add a bad path test for local sockets
• adjust test_bad_connection MemcachePool creation call to work (the new opener pulls host during creation, but it was being changed inside the test originally)

Are there changes in behavior for the user?

I've added a path kwarg they can call, but all per-existing code should still work as is.

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes

[Kropyls]

ouch. @Dreamsorcerer , if I'm reading the CI results right it looks like the tests failed because the memcached docker container (Here I believe: https://github.com/jkeys089/actions-memcached/blob/master/entrypoint.sh) does not have a local socket service exposed. Makes sense, and sorry I missed that :(

Problem is, one memcached service can't listen on both an IP/port and a socket at the same time. That isn't a problem for me when I test locally, I just make 2 containers or run 2 services. Not sure if you want me adding that to your CI code though? I'm happy to work up something if you like though. What I'd probably do is add a action-memcached-local that looks similar to the actions-memcached setup, and call that just before the "run tests" code in ci.yaml.

If you're good with that approach I'll get something in shortly. I'm also open to any alternate methods you think would be better as well?

[Dreamsorcerer]

Feel free to make any suggestions. I've not had a chance to look at it, and I didn't set it up originally either. There may be better a better approach available today.

[Kropyls]

Alright @Dreamsorcerer , lets hope I got it right this time! The only "gotcha" here is that you now have an extra dep in the ci file on Kropyls/actions-memcached-local. Maybe I'm just paranoid, but I don't love depending on another repo to always be there for my jobs to work.

Of course - I don't plan on going anywhere, nor taking this down ever, but I still feel obligated to bring the point up for your consideration so you can decide what you're comfortable with.

Here's hoping the job runs successfully now though!

[Dreamsorcerer]

Running it through Claude quickly, it has this suggestion:

Fix: drop docker entirely, install memcached on the runner

The runner is a full Ubuntu VM; the workflow already does sudo apt-get install libmemcached-dev. Replace both actions with:

      - name: Run memcached                                                                                                                                                                                          
        run: |                                                                                                                                                                                                       
          sudo apt-get install -y memcached                                                                                                                                                                          
          sudo systemctl disable --now memcached                                                                                                                                                                     
          memcached -d -p 11211 -l 127.0.0.1                                                                                                                                                                         
          memcached -d -s /tmp/memcached.sock                                                                                                                                                                        

• systemctl disable --now: the Debian package auto-starts a unit on 127.0.0.1:11211; stopping it makes the two explicit instances deterministic instead of relying on distro packaging behavior. (Relying on the
  packaged service for TCP and only spawning the socket instance also works, if you prefer 2 lines.)
• No -a 0777 needed: the socket's default 0700 mask is fine because pytest runs as the same user that spawned memcached. Kropyls' action needs 0777 only because the container's memcache user ≠ host runner user.
• Kills both supply-chain deps and the docker indirection in one move.

Required PR-side alignment

The PR's mcache_unix_server fixture hardcodes /var/run/memcached/memcached.sock — that path exists only because his action chmod 777s a root-owned dir. With the host install, use a runner-writable path and match
it in the fixture:

  @pytest.fixture(scope='session')                                                                                                                                                                                   
  def mcache_unix_server(session_id: str) -> UnixServerParams:                                                                                                                                                       
      return mcache_unix_server_actual("/tmp/memcached.sock")                                                                                                                                                        

(Or mirror the existing --memcached pytest option with a --memcached-socket option so the path isn't hardcoded at all.)

[Kropyls]

@Dreamsorcerer , that arguement makes sense to me for the most part. It cuts back the dependance on externalities, which is great. The only reason I hadn't writtem the socket to /tmp directly initially was systemd defaults to privatetmp=true for the memcached unit file.

I added a comment in conftest.py about that so anyone testing locally can hopefully see it and be aware of that issue, if they are even using systemd to test it.

I do think the benefits are worth that small cost in this case though - I sent over a new commit with those changes incorperated.

[codecov]

Codecov Report

❌ Patch coverage is 94.73684% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.77%. Comparing base (b5543d0) to head (465017b).
⚠️ Report is 41 commits behind head on master.

Files with missing lines	Patch %	Lines
tests/conftest.py	85.71%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #504      +/-   ##
==========================================
+ Coverage   92.32%   95.77%   +3.44%     
==========================================
  Files           9        9              
  Lines         769      781      +12     
  Branches       42       43       +1     
==========================================
+ Hits          710      748      +38     
+ Misses         48       22      -26     
  Partials       11       11              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Kropyls]

@Dreamsorcerer , I'm not sure I'm understanding what the codecov report is saying? It seems primarily mad about the docker functions in conftest.py but it doesn't seem to say (that I can see) what it thinks is problematic with the lines? Am I missing something here?

Thanks.

[Dreamsorcerer]

It seems primarily mad about the docker functions in conftest.py but it doesn't seem to say (that I can see) what it thinks is problematic with the lines?

None of that code is being executed. Not sure why I missed it earlier, but it's not used anywhere, so why is it being added?

[Dreamsorcerer]

Looks like the original mcache_server_docker() is also unused, so we could remove that too in a separate PR.

[Kropyls]

ah, yeah. Makes sense now. The reason I added it (and several of the other items you noted) basically links back to how in my initial PR how I was following the initial pattern for IP-based logic where it existed. Since there was an initial docker function, I added a unix one, presuming there was a reason the code had been left there (maybe a forward-looking one). Testing both the docker functions was interesting because of this, I ended up building a shim outside the library for both vanilla and unix-based docker functions :P

Taking it out would make things simpler and fix a couple things at once, so I'm happy to do that if you don't think keeping it has merit. I was expecting that might be the direction you wanted to take things when I opened PR but I wanted to at least have the matching docker function written so that the decision could be made while having the function to evaluate. Taking away is easier than adding and all that.

[Dreamsorcerer]

I assume the original has been unused for many years, so I think it's safe to remove and cleanup the others later.

[Kropyls]

@Dreamsorcerer , I corrected all of your comments/concerns that I saw at this point. The most significant change was dropping the docker functions in conftest.py, as we discussed. Otherwise I dropped the self._unix values per your comments because the self._port now carries that information, so it was redundant (even if it did make it easy to read in the debugger)

Please have a looks when you are able and give me any additional feedback you have. I have a feeling we are getting pretty close to the finish line here!

[Kropyls]

Looks like some kind of server error on that last run. Mind giving it a re-run @Dreamsorcerer ? Thanks