Skip to content

fix: Unmask exceptions in listener retries#4615

Merged
steve-chavez merged 1 commit intoPostgREST:mainfrom
mkleczek:unmask-exceptions-in-listener-retries
Jan 28, 2026
Merged

fix: Unmask exceptions in listener retries#4615
steve-chavez merged 1 commit intoPostgREST:mainfrom
mkleczek:unmask-exceptions-in-listener-retries

Conversation

@mkleczek
Copy link
Copy Markdown
Collaborator

forkFinally calls error handler with exceptions masked. Our handleFinally does not restore exception state before calling retryingListener so after first error the whole listener is running with exceptions masked.
This patch ensures handleFinally is run with exceptions unmasked.

@mkleczek mkleczek force-pushed the unmask-exceptions-in-listener-retries branch from 0adc283 to 6a8b7d8 Compare January 25, 2026 19:46
@mkleczek mkleczek force-pushed the unmask-exceptions-in-listener-retries branch 5 times, most recently from 0ec31f5 to ab47d94 Compare January 28, 2026 06:15
Comment on lines +64 to +65
-- TODO is masking really necessary here? We unmask the onError handler anyway...
void $ mask $ \unmask -> handle (unmask . onError) $ unmask $ do
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.

Right, I was thinking the same.. can we avoid the mask/unmask?

FWIW, forkFinally code is simple:

forkFinally :: IO a -> (Either SomeException a -> IO ()) -> IO ThreadId
forkFinally action and_then =
  mask $ \restore ->
    forkIO $ try (restore action) >>= and_then

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Right, I was thinking the same.. can we avoid the mask/unmask?

Fixed.

I've also changed return type of retryingListen to IO Void to make sure no accidental loop exit happens.

Updated changeling as well and I think it is now in a committable shape.

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.

Nice! forever also makes the code look much clearer 💯

@mkleczek mkleczek force-pushed the unmask-exceptions-in-listener-retries branch 2 times, most recently from 00c9526 to a08908c Compare January 28, 2026 20:25
forkFinally calls error handler with exceptions masked. Our handleFinally does not restore exception state before calling retryingListener so after first error the whole listener is running with exceptions masked.
This patch ensures handleFinally is run with exceptions unmasked.
@mkleczek mkleczek force-pushed the unmask-exceptions-in-listener-retries branch from a08908c to 474a959 Compare January 28, 2026 20:39
@steve-chavez steve-chavez merged commit 73a4655 into PostgREST:main Jan 28, 2026
32 checks passed
@postgrest-ci
Copy link
Copy Markdown

postgrest-ci bot commented Jan 29, 2026

Backport failed for v14, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin v14
git worktree add -d .worktree/backport-4615-to-v14 origin/v14
cd .worktree/backport-4615-to-v14
git switch --create backport-4615-to-v14
git cherry-pick -x 73a465501e9574fbd77505fb807a461392f3c52e

@taimoorzaeem
Copy link
Copy Markdown
Member

taimoorzaeem commented Jan 29, 2026

Looks like the backport workflow failed due to conflicts caused by code changes in #4617 and #4618.

@steve-chavez
Copy link
Copy Markdown
Member

Fixed the conflict and pushed in 16c7671

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

Development

Successfully merging this pull request may close these issues.

3 participants