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

fix: upgrade hasql-notifications to show error #3324

Merged
merged 1 commit into from
Mar 13, 2024
Merged

fix: upgrade hasql-notifications to show error #3324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- #3224, Return status code 406 for non-accepted media type instead of code 415 - @wolfgangwalther
- #3160, Fix using select= query parameter for custom media type handlers - @wolfgangwalther
- #3237, Dump media handlers and timezones with --dump-schema - @wolfgangwalther
- #3323, Don't hide error on LISTEN channel failure - @steve-chavez
- #3323, #3324, Don't hide error on LISTEN channel failure - @steve-chavez

### Deprecated

Expand Down
16 changes: 14 additions & 2 deletions nix/overlays/haskell-packages.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,18 @@ let
#
# To temporarily pin unreleased versions from GitHub:
# <name> =
# prev.callCabal2nixWithOptions "<name>" (super.fetchFromGitHub {
# lib.dontCheck (prev.callCabal2nixWithOptions "<name>" (super.fetchFromGitHub {
# owner = "<owner>";
# repo = "<repo>";
# rev = "<commit>";
# sha256 = "<sha256>";
# }) "--subpath=<subpath>" {};
# }) "--subpath=." {});
Comment on lines -22 to +27
Copy link
Member

Choose a reason for hiding this comment

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

I don't think those changes make sense for the template here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, why? I'm always lost when I see that <subpath> and lib.dontCheck is almost always the wanted behavior.

Copy link
Member

Choose a reason for hiding this comment

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

While working on compiling with GHC 9.8 inside nix on a local branch, I have added plenty of those in the last couple of weeks. My experience so far:

  • lib.dontCheck was used not very often - and even if it was, it should not be the default as in "copy & paste comes with it", but should be added once those tests actually fail.
  • Having --subpath=. hides the fact that this is configurable and not some required default setting. I had quite a few cases where I needed a subpath other than .. On the other hand, if <subpath> is in there, you might need to think about it, yes, but the solution will be obvious, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok cool. Addressed on #3326

#
# To fill in the sha256:
# update-nix-fetchgit nix/overlays/haskell-packages.nix
#
# Nowadays you can just delete the sha256 attribute above and nix will assume a fake sha.
# Once you build the derivation it will suggest the correct sha.

configurator-pg =
prev.callHackageDirect
Expand All @@ -48,6 +51,15 @@ let

hasql-pool = lib.dontCheck prev.hasql-pool_0_10;

hasql-notifications = lib.dontCheck (prev.callHackageDirect
{
pkg = "hasql-notifications";
ver = "0.2.1.0";
sha256 = "sha256-MEIirDKR81KpiBOnWJbVInWevL6Kdb/XD1Qtd8e6KsQ=";
Comment on lines +56 to +58
Copy link
Member

Choose a reason for hiding this comment

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

If we want to consider this as part of our bug fix, we should bump the lower bounds in postgrest.cabal to 0.2.1.0, too. This means we'd need to update stack.yaml with the extra-dep, too.

Copy link
Member

Choose a reason for hiding this comment

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

Additionally the index-state in cabal.project.freeze needs to be updated, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, those are too easy to miss. Since we have cabal and stack running on CI, shouldn't those give an error?

Copy link
Member

Choose a reason for hiding this comment

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

They won't be missed - if you take the first step before the second. Adjusting the package overlay for nix is the second step. If you need to bump dependencies, you should always go to postgrest.cabal first. Once you update the lower bound in there, everything else, including nix, will fail. But you need to update postgrest.cabal there is no way around that.

}
{ }
);

};
in
{
Expand Down
4 changes: 2 additions & 2 deletions src/PostgREST/AppState.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,11 +492,11 @@ listener appState observer = do
exitFailure
where
handleFinally dbChannel False err = do
observer $ DBListenerFailNoRecoverObs dbChannel err
observer $ DBListenerFailRecoverObs False dbChannel err
killThread (getMainThreadId appState)
handleFinally dbChannel True err = do
-- if the thread dies, we try to recover
observer $ DBListenerFailRecoverObs dbChannel err
observer $ DBListenerFailRecoverObs True dbChannel err
putIsListenerOn appState False
-- assume the pool connection was also lost, call the connection worker
connectionWorker appState
Expand Down
16 changes: 8 additions & 8 deletions src/PostgREST/Observation.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ module PostgREST.Observation
) where

import qualified Data.ByteString.Lazy as LBS
import qualified Data.Text as T
import qualified Data.Text.Encoding as T
import qualified Hasql.Connection as SQL
import qualified Hasql.Pool as SQL
Expand Down Expand Up @@ -38,8 +39,7 @@ data Observation
| ConnectionPgVersionErrorObs SQL.UsageError
| DBListenerStart Text
| DBListenerFail Text SQL.ConnectionError
| DBListenerFailNoRecoverObs Text (Either SomeException ())
| DBListenerFailRecoverObs Text (Either SomeException ())
| DBListenerFailRecoverObs Bool Text (Either SomeException ())
| ConfigReadErrorObs
| ConfigReadErrorFatalObs SQL.UsageError Text
| ConfigReadErrorNotFatalObs SQL.UsageError
Expand Down Expand Up @@ -85,10 +85,8 @@ observationMessage = \case
"Listening for notifications on the " <> channel <> " channel"
DBListenerFail channel err -> do
"Could not listen for notifications on the " <> channel <> " channel. " <> show err
DBListenerFailNoRecoverObs channel err ->
showListenerError err <> ". Automatic recovery disabled on the " <> channel <> " channel"
DBListenerFailRecoverObs channel err ->
showListenerError err <> ". Retrying listening for notifications on the " <> channel <> " channel.."
DBListenerFailRecoverObs recover channel err ->
"Could not listen for notifications on the " <> channel <> " channel. " <> showListenerError err <> (if recover then " Retrying listening for notifications.." else mempty)
ConfigReadErrorObs ->
"An error ocurred when trying to query database settings for the config parameters"
ConfigReadErrorFatalObs usageErr hint ->
Expand All @@ -112,5 +110,7 @@ observationMessage = \case
jsonMessage err = T.decodeUtf8 . LBS.toStrict . Error.errorPayload $ Error.PgError False err

showListenerError :: Either SomeException () -> Text
showListenerError (Left e) = show e
showListenerError (Right _) = "Failed getting notifications" -- this should not happen as the listener will never finish with a Right result
showListenerError (Right _) = "Failed getting notifications" -- should not happen as the listener will never finish (hasql-notifications uses `forever` internally) with a Right result
showListenerError (Left e) =
let showOnSingleLine txt = T.intercalate " " $ T.filter (/= '\t') <$> T.lines txt in -- the errors from hasql-notifications come intercalated with "\t\n"
showOnSingleLine $ show e
Loading