search

typelevel / natchez

functional tracing for cats
MIT License
322 stars 77 forks source link

`ExitCase` lost in several places #980

Closed kubukoz closed 4 months ago

kubukoz commented 4 months ago

Hi!

I noticed that allocated is used several times instead of allocatedCase. If I got it right, this would mean that the ExitCase that the finalizer of a resource receives will always be ExitCase.Succeeded, even if wrapped in another Resource.apply.

The relevant spots are:

https://github.com/typelevel/natchez/blob/d1513b862d615c491a6bbb9ebfe28211513b1492/modules/mtl/shared/src/main/scala/LocalTrace.scala#L39

https://github.com/typelevel/natchez/blob/d1513b862d615c491a6bbb9ebfe28211513b1492/modules/core/shared/src/main/scala/Trace.scala#L180

(there are actually more in that file)

I believe for a correct implementation these should be replaced with the case-aware variants. Otherwise, the resources being wrapped will never be closed with a Canceled/Errored exit.

kubukoz commented 4 months ago

I suppose the code was written back in the day of CE2, when we didn't have allocatedCase. Also, instead of Resource.apply we should probably be using applyFull and wrapping the underlying resource in cancelable