Skip to content

Conversation

@maltoe
Copy link
Collaborator

@maltoe maltoe commented Jan 5, 2026

The docs claim

We decided that we wanted to have atom or string keys for better readability.

... yet when I try it with an atom, I get

iex(3)> MyApp.Repo.advisory_xact_lock(:foo)
** (ArgumentError) errors were found at the given arguments:

  * 1st argument: not an iodata term

    (erts 15.2.3) :erlang.iolist_to_binary(:foo)
    (crypto 5.5.3) crypto.erl:1039: :crypto.hash/2
    (bitcrowd_ecto 1.0.0) lib/bitcrowd_ecto/repo.ex:258: BitcrowdEcto.Repo.advisory_xact_lock/2
    iex:3: (file)

@maltoe maltoe requested a review from agatheblues January 5, 2026 11:41
Copy link
Member

@andreasknoepfle andreasknoepfle left a comment

Choose a reason for hiding this comment

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

A test would also be really nice, but I realise this function is yet untested...

@maltoe
Copy link
Collaborator Author

maltoe commented Jan 5, 2026

A test would also be really nice, but I realise this function is yet untested...

Could do just for testing the API, but I've tried before to test locks in unit tests and it's not trivial with actual concurrency. We could assert on the presence of a locktype = 'advisory' in pg_locks, but not sure if that helps much? It's also prone to race consditions with other tests, so would need async: false?

@andreasknoepfle
Copy link
Member

A test that the function does not crash would maybe be enough. Would have worked to cover the case for atoms.

@maltoe
Copy link
Collaborator Author

maltoe commented Jan 5, 2026

@andreasknoepfle added some tests

@maltoe maltoe force-pushed the fix/advisory-xact-lock-should-accept-atoms branch from 15d12c6 to 4ef46d5 Compare January 5, 2026 14:15
The docs claim

> We decided that we wanted to have atom or string keys for better readability.

... yet when I try it with an atom, I get

```elixir
iex(3)> MyApp.Repo.advisory_xact_lock(:foo)
** (ArgumentError) errors were found at the given arguments:

  * 1st argument: not an iodata term

    (erts 15.2.3) :erlang.iolist_to_binary(:foo)
    (crypto 5.5.3) crypto.erl:1039: :crypto.hash/2
    (bitcrowd_ecto 1.0.0) lib/bitcrowd_ecto/repo.ex:258: BitcrowdEcto.Repo.advisory_xact_lock/2
    iex:3: (file)
```
@maltoe maltoe force-pushed the fix/advisory-xact-lock-should-accept-atoms branch from 4ef46d5 to 20f39f8 Compare January 7, 2026 08:40
@andreasknoepfle andreasknoepfle merged commit ba34b5e into main Jan 7, 2026
1 check passed
@andreasknoepfle andreasknoepfle deleted the fix/advisory-xact-lock-should-accept-atoms branch January 7, 2026 09:07
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.

3 participants