Skip to content

refactor: improve multi_socket_cb implementation#260

Merged
steve-chavez merged 8 commits into
masterfrom
rs/fix-segfault-on-mac
Jul 7, 2026
Merged

refactor: improve multi_socket_cb implementation#260
steve-chavez merged 8 commits into
masterfrom
rs/fix-segfault-on-mac

Conversation

@imor

@imor imor commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

This PR improves the multi_socket_cb implementation for both kqueue and epoll by doing the following:

For kqueue:

  • Fixes the leaks in the filters because the sock_info->action was not being updated. This also lead to a use-after-free bug and sometimes made the tests on macOS fail due to a segfault. All these bugs are fixed.
  • Correctly checks the return value of curl_multi_assign and raises a Postgres error if the call fails.

For epoll:

  • Correctly checks the return value of curl_multi_assign and raises a Postgres error if the call fails.
  • Correctly handle the case when what == CURL_POLL_INOUT by setting the right EPOLLIN and EPOLLOUT combination in ev.events member.
  • Sets a sentinel value in the call to curl_multi_assign the first time multi_socket_cb is called for a socket fd and removes it when what == CURL_POLL_REMOVE. This clarifies the intent of the curl_multi_assign call because the older code assignments to socket_exists variable were never read and were confusing.

@steve-chavez

steve-chavez commented Jul 2, 2026

Copy link
Copy Markdown
Member

@imor Can we prefix this PR as refactor: instead of fix:? Two reasons:

  • It doesn't need a release since Macos is not used for production. Otherwise it could be confusing for packagers like supabase/postgres
  • Macos compatibility was only provided for development purposes.

@imor

imor commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@steve-chavez yeah I intend to clean this PR up. I added this as a draft so that I don't forget. I only needed this change to run the tests on my local setup.

@imor imor changed the title fix: segfault when too many confurrent request were sent on macos refactor: improve multi_socket_cb implementation Jul 7, 2026
@imor imor marked this pull request as ready for review July 7, 2026 10:00

@steve-chavez steve-chavez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@imor Looks good! On second thought I'll change this to a fix: commit since it also changes the logic for epoll. That way the change clearly shows on the release (in case if something goes wrong).

@steve-chavez steve-chavez merged commit 7eea125 into master Jul 7, 2026
36 of 45 checks passed
@steve-chavez

Copy link
Copy Markdown
Member

A further improvement could be to run the loadtest on macos, that should prove pg_net works the same there

loadtest:
runs-on: ubuntu-latest
strategy:
matrix:
params: [ {reqs: 10000, batch: 200}, {reqs: 20000, batch: 400}, {reqs: 40000, batch: 800} ]
steps:
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
- name: Install Nix
uses: cachix/install-nix-action@c134e4c9e34bac6cab09cf239815f9339aaaf84e # v31.5.1
- name: Use Cachix Cache
uses: cachix/cachix-action@3ba601ff5bbb07c7220846facfa2cd81eeee15a1 # v16
with:
name: nxpg
authToken: ${{ secrets.CACHIX_AUTH_TOKEN }}
- name: Build
run: nix-shell --run "xpg build"
- name: Run load test
run: |
nix-shell --run "net-loadtest ${{ matrix.params.reqs }} ${{ matrix.params.batch }}" >> "$GITHUB_STEP_SUMMARY"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants