Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Possible bug in :cachex_notify event handling? #325

Closed
feld opened this issue Jan 23, 2024 · 1 comment · Fixed by #327
Closed

Possible bug in :cachex_notify event handling? #325

feld opened this issue Jan 23, 2024 · 1 comment · Fixed by #327
Labels
bug
Milestone
v4.0.0

Comments

@feld
Copy link
Contributor

feld commented Jan 23, 2024 •
edited
Loading

cachex/lib/cachex/hook.ex

Lines 165 to 182 in 5b8983e

def handle_info({:cachex_notify, {event, result}}, state) do
case timeout() do
nil ->
{:ok, new_state} = handle_notify(event, result, state)
{:noreply, new_state}
val ->
task =
Task.async(fn ->
handle_notify(event, result, state)
end)
case Task.yield(task, val) || Task.shutdown(task) do
{:ok, {:ok, new_state}} -> {:noreply, state}
nil -> {:noreply, state}
end
end
end

I was trying to refactor this for a dialyzer error that seemed like it was mostly benign, but then I realized there may be a logic error here which (surprisingly) neither the compiler nor dialyzer picked up. Follow this logic:

  1. timeout() returns a non-nil value
  2. a Task is launched calling handle_notify/3
  3. If the Task yields in time, we have the case statement
  4. The task yielded a value! {:ok, {:ok, new_state}} -> {:noreply, state} ... but we returned the old state anyway just like if the Task didn't yield in time? Why?
@whitfin
Copy link
Owner

whitfin commented Jan 23, 2024

Yes, this is a bug - although like you say I highly doubt this ever affected anyone (and it dates back six years). Feel free to PR a fix!

@whitfin whitfin added the bug label Jan 23, 2024
@whitfin whitfin added this to the v3.7.0 milestone Jan 23, 2024
feld added a commit to feld/cachex that referenced this issue Jan 26, 2024
@feld
Return updated state in the cache notification event when the registe…
845ed33 
…red Hook is defined with a timeout value

Fixes whitfin#325
@feld feld mentioned this issue Jan 26, 2024
Merged
whitfin pushed a commit that referenced this issue Jan 29, 2024
@feld @whitfin
Return updated state in the cache notification event when the registe…
a876544 
…red Hook is defined with a timeout value

Fixes #325
@whitfin whitfin modified the milestones: v3.7.0, v4.0.0 Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
v4.0.0
Development

Successfully merging a pull request may close this issue.

Return updated state in the cache notification event feld/cachex
2 participants
@whitfin @feld