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.

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

flushTBQueue is broken since 2.5.2.0 #76

Open
nikita-volkov opened this issue Oct 19, 2023 · 1 comment
Open

flushTBQueue is broken since 2.5.2.0 #76

nikita-volkov opened this issue Oct 19, 2023 · 1 comment

Comments

@nikita-volkov
Copy link
Contributor

It acts weirdly. Causes random halts and etc.

One issue that is easily reproducible is that the following test fails:

    describe "TBQueue" do
      describe "flushTBQueue" do
        it "Affects the length" do
          queue <- newTBQueueIO 100
          length <- atomically $ do
            writeTBQueue queue 1
            writeTBQueue queue 2
            flushTBQueue queue
            lengthTBQueue queue
          shouldBe length 0

Replacing the implementation with the following suboptimal one can serve as a quick fix for the time being:

flushTBQueue :: TBQueue a -> STM [a]
flushTBQueue queue =
  go []
  where
    go !acc = do
      element <- tryReadTBQueue queue
      case element of
        Just element -> go $ element : acc
        Nothing -> return $ reverse acc

I guess the bug was introduced in #70. So @konsumlamm please take a look.


@bgamari @simonmar @hvr

We need stricter quality assurance standards for this package. Bugs in packages as central as this can have very dire impact on the stability of the whole ecosystem and the reputation of the language. There's 13496 indirect dependencies just on Hackage, meaning that virtually any application can get affected by bugs in this package. Another issue is that people don't expect bugs from such central packages, e.g., I've lost quite some time debugging this simply because I could not believe that the bug would come from "stm".

I suggest that rewrites should be done with extreme caution and should be exhaustively covered with tests. As I see #70 came with 0 tests.

@nikita-volkov
Copy link
Contributor Author

Here's an idea on how the new implementation can be tested. The old implementation can be copied to a test suite which will generate random actions and execute them on both implementations and compare the results. Thus we can ensure that the new implementation behaves the same as the old one.

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 a pull request may close this issue.

1 participant