Add more powerful cleanup functions by harendra-kumar · Pull Request #3060 · composewell/streamly

harendra-kumar · 2025-06-25T20:01:40Z

So that we can release resources any time.
Fix atomicity issue during allocation.
Rename the APIs.

Copilot

Pull Request Overview

This PR renames and enhances resource cleanup APIs to provide deterministic, on-demand cleanup without relying on GC, fixes allocation atomicity, and updates tests accordingly.

Introduce withBracketIO/withBracketIOM and withFinallyIO/withFinallyIOM to replace cleanupIO/cleanupEffectIO.
Update tests to use the new withFinally* functions.
Improve internal implementation with IORef+Map for multiple registrations and atomic modifications.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
test/Streamly/Test/Data/Stream/Exception.hs	Swapped `cleanupIO`/`cleanupEffectIO` calls to `withFinallyIO`/`withFinallyIOM`
core/src/Streamly/Internal/Data/Stream/Exception.hs	Removed old `cleanup` exports, added `withBracket` & `withFinally*` implementations, updated docs

Comments suppressed due to low confidence (4)

core/src/Streamly/Internal/Data/Stream/Exception.hs:24

Removing cleanupIO from the export without a deprecation or alias may break backward compatibility. Consider adding a deprecated alias forwarding calls to withFinallyIO before fully removing it.

    , bracketIO

core/src/Streamly/Internal/Data/Stream/Exception.hs:25

Removing cleanupEffectIO from the export without a deprecation or alias may break backward compatibility. Consider adding a deprecated alias forwarding calls to withFinallyIOM before fully removing it.

    , withBracketIO

core/src/Streamly/Internal/Data/Stream/Exception.hs:406

The doc for withFinallyIOM references withBracketIOM. It should instead mention withFinallyIOM to accurately describe when the hooks are run.

-- would run automatically when 'withBracketIOM' ends or if an exception occurs

core/src/Streamly/Internal/Data/Stream/Exception.hs:417

[nitpick] The suffix ‘IOM’ is ambiguous; consider a more descriptive name such as withFinallyEffect or including Effect to clearly distinguish it from withFinallyIO.

withFinallyIOM :: (MonadIO m, MonadCatch m) =>

So that we can release resources any time. Fix atomicity issue during allocation. Rename the APIs.

* Fix manual freeing of resources, add the missing "free" * Add "forall" to types so that different types of resources can be alloced/freed. * Make allocation thread safe and exception safe

Fixes some issues in Stream.withAllocateIO.

adithyaov · 2025-07-07T08:06:07Z

+
+-- | Resources with Priority1 are freed before Priority2. This is especially
+-- introduced to take care of the case where we need to free channels, so that
+-- all the workers of the channel are cleanup before we free the resources


cleanup -> cleaned up

adithyaov · 2025-07-07T08:21:44Z

-            release = do
-                f <- atomicModifyIORef ref modify
-                sequence_ f
+    -- XXX can we ensure via GC that the resources that we are freeing are all


So we don't support parallel release yet.
PS: I'm reviewing the commits one by one. This commit may be obsolete.

adithyaov · 2025-07-07T08:26:03Z

+                -- IMPORTANT: do not use interruptible operations in this
+                -- critical section. Even putStrLn can make tests fail.


What exactly are interruptible operations?
Operations that allow the thread to sleep?

See Control.Exception in base.

adithyaov · 2025-07-07T08:26:38Z

+                    -- restoring exceptions makes it non-atomic, tests fail.
+                    -- Can use allowInterrupt in "free" if desired.


I'm unable to follow this.

adithyaov · 2025-07-07T08:35:36Z

+        -- XXX free them atomically, even if another release executes in
+        -- parallel.


By this statement, you assume that either this atomic free will happen first before the parallel release or the parallel release will happen first, and this won't.

adithyaov · 2025-07-07T08:36:41Z

+                -- We run it without an exception mask unlike withAllocateIO in
+                -- the Streamly.Control.Exception module. We can add it if
+                -- needed.
+                res <- atomicModifyIORef' ref modify


So, async exceptions can trigger this.

adithyaov · 2025-07-07T08:42:05Z

 --
 setReleaseCb :: AllocateIO -> Config -> Config
-setReleaseCb f cfg = cfg { _release = Just (register f) }
+setReleaseCb f cfg = cfg { _release = Just (registerWith f Priority1) }


The callback registered in the channel config will always be called first, and then the other callbacks.
The user has to be wary about resource cleanup and decide what should go where.

only streamly developers need to worry about the priority. For normal users there is one priority.

adithyaov · 2025-07-07T08:46:12Z

+-- only one will succeed others will block until it finishes.
+--
+cleanupChan :: Channel m a -> String -> IO ()
+cleanupChan chan reason = do


There will always be a major GC performed before the channel is marked Stopped.
It may not be required in many cases where there isn't any resource allocation.
Is it possible to performMajorGC only when required?

performMajorGC is done only in inspect mode so that logs come properly.

adithyaov · 2025-07-07T08:55:52Z

+-- We use GC based hooks in Stream.bracketIO' so there could be async threads
+-- spawned by GC, releasing resources concurrently with us. For that reason we
+-- need to make sure that the "release" in the bracket end action is executed
+-- only once in that case.
+--


Is it possible to remove GC hooks? Now that we have prompt cleanup why do we still have them?

to cleanup even before the "prompt" release happens. prompt release can be later than GC.

adithyaov · 2025-07-07T08:57:32Z

-- all the workers of the channel are cleanup before we free the resources
-- allocated by the workers of the channel. Otherwise we might free the
-- resources and workers may be trying to use them and start misbehaving.
+-- | Resources with 'Priority1' are freed before 'Priority2'. Priority is


Can you also give a small example on how this helps in freeing concurrency channels?

A stream is being evaluated concurrently by multiple workers, each worker allocates some resource and is working with those resources. An interrupt comes and cleanup starts. Assume, we cleanup the resources of the workers first and then the worker. Now the worker is still working so it might crash or misbehave of because of the resource slipping from under it. So we need ensure that we call the cleanup of the workers before we cleanup their resources.

adithyaov · 2025-07-07T09:01:00Z

@harendra-kumar Although I have a few questions, I think I understand the motivation behind these changes.

harendra-kumar requested review from adithyaov and Copilot June 25, 2025 20:01

Copilot AI reviewed Jun 25, 2025

View reviewed changes

composewell deleted a comment from Copilot AI Jun 25, 2025

adithyaov reviewed Jun 25, 2025

View reviewed changes

Comment thread core/src/Streamly/Internal/Data/Stream/Exception.hs Outdated

harendra-kumar force-pushed the prompt-cleanup1 branch from b0f6240 to d367bcd Compare June 28, 2025 01:00

adithyaov approved these changes Jun 29, 2025

View reviewed changes

harendra-kumar force-pushed the prompt-cleanup1 branch from 53f67c0 to b8f1ae8 Compare June 29, 2025 20:57

harendra-kumar added 6 commits July 6, 2025 09:49

Add Streamly.Test.Data.Stream.Exception to hie

a08e94a

Add more powerful cleanup functions

6d5bcc9

So that we can release resources any time. Fix atomicity issue during allocation. Rename the APIs.

Make enhancements to prompt resource cleanup

8c1c3c5

* Fix manual freeing of resources, add the missing "free" * Add "forall" to types so that different types of resources can be alloced/freed. * Make allocation thread safe and exception safe

Use Allocate and Register types

8253c15

Move the effect based resource ops to Exception module

0b7f343

Remove withRegisterIO, add bracketIO'

5cb8a98

harendra-kumar force-pushed the prompt-cleanup1 branch 2 times, most recently from 9782b7a to 23f119c Compare July 6, 2025 08:39

harendra-kumar added 8 commits July 7, 2025 09:28

Fix resource management in concurrent streams

2bf1f73

Use code from Exception.withAllocateIO in Stream.withAllocateIO

44b320a

Fixes some issues in Stream.withAllocateIO.

Rename AllocateIO to AcquireIO

345e98f

Rename setReleaseCb to useAcquire

b063a6f

Fix documentation for resource management ops

74a37e4

Fix docspec for resource management ops

4865e84

Add module Streamly.Control.Exception

8a5c0ae

Expose useAcquire, clearAcquire

24468aa

harendra-kumar force-pushed the prompt-cleanup1 branch from eb438cc to 24468aa Compare July 7, 2025 04:06

adithyaov reviewed Jul 7, 2025

View reviewed changes

harendra-kumar merged commit 24468aa into master Jul 7, 2025
19 of 22 checks passed

harendra-kumar deleted the prompt-cleanup1 branch July 9, 2025 20:49

		-- IMPORTANT: do not use interruptible operations in this
		-- critical section. Even putStrLn can make tests fail.

		-- restoring exceptions makes it non-atomic, tests fail.
		-- Can use allowInterrupt in "free" if desired.

		-- XXX free them atomically, even if another release executes in
		-- parallel.

Conversation

harendra-kumar commented Jun 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adithyaov Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adithyaov Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adithyaov Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adithyaov commented Jul 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adithyaov Jul 7, 2025 •

edited

Loading

adithyaov Jul 7, 2025 •

edited

Loading

adithyaov Jul 7, 2025 •

edited

Loading