nspcc-dev/neofs-sdk-go

Fork 0

mirror of https://github.com/nspcc-dev/neofs-sdk-go.git synced 2026-03-01 04:29:18 +00:00

client: Stats of failed object `GET/RANGE/SEARCH` are submitted only on `Close` #251

New issue

Open

opened 2025-12-28 18:07:34 +00:00 by sami · 7 comments

sami commented

2025-12-28 18:07:34 +00:00

Owner

Originally created by @cthulhu-rider on GitHub (Dec 9, 2024).

currently, server-streamed ops call stat handler only when user calls Close. But for such ops Close is only used when user is no longer interested in the stream. Unlike PUT when user signals that data is finished via Close. For example, following code looks completely OK:

r, err := c.ObjectSearchInit(ctx, anyCID, anyValidSigner, anyValidOpts)
for err == nil {
	_, err = r.Read([]oid.ID{{}})
}
return err // io.EOF or failure

Current Behavior

-stats

Expected Behavior

+stats

Possible Solution

do not require calling Close. Submit stats whenever stream is finished (io.EOF==OK!). Update docs

Steps to Reproduce

see exec statistics cases from #641

Regression

quite possible, but i havent checked

Your Environment

Version of the product used: 335d9fe90f

Originally created by @cthulhu-rider on GitHub (Dec 9, 2024). currently, server-streamed ops call stat handler only when user calls `Close`. But for such ops `Close` is only used when user is no longer interested in the stream. Unlike PUT when user signals that data is finished via Close. For example, following code looks completely OK: ```go r, err := c.ObjectSearchInit(ctx, anyCID, anyValidSigner, anyValidOpts) for err == nil { _, err = r.Read([]oid.ID{{}}) } return err // io.EOF or failure ``` ## Current Behavior -stats ## Expected Behavior +stats ## Possible Solution do not require calling `Close`. Submit stats whenever stream is finished (io.EOF==OK!). Update docs ## Steps to Reproduce * see `exec statistics` cases from #641 ## Regression quite possible, but i havent checked ## Your Environment * Version of the product used: 335d9fe90f24494944c2888ef385f06f5f81dcfc

sami added the

bug

client

blocked

labels

2025-12-28 18:07:34 +00:00

sami commented

2025-12-28 18:07:35 +00:00

Author

Owner

@roman-khimov commented on GitHub (Dec 9, 2024):

That was a deliberate choice because previously this statistics was "collected" for each r.Read() call. Tying it to Close() was the easiest way to go. The main question here --- is Close() optional? It's not to me, we're providing io.ReaderCloser, most of the time people are supposed to call Close() to release some resource in this case.

@roman-khimov commented on GitHub (Dec 9, 2024): That was a deliberate choice because previously this statistics was "collected" for each `r.Read()` call. Tying it to `Close()` was the easiest way to go. The main question here --- is `Close()` optional? It's not to me, we're providing `io.ReaderCloser`, most of the time people are supposed to call `Close()` to release some resource in this case.

sami commented

2025-12-28 18:07:35 +00:00

Author

Owner

@cthulhu-rider commented on GitHub (Dec 10, 2024):

@roman-khimov i meant this in terms of stats. We can submit real stats when stream is done and Read() returns an error regardless of closing. This is what i actually propose. I just noticed Failure reason can be received via Close. in docs, and this is not quire true. After Read() returns an error, Close() is no-op and returns the same

overall, i agree that closer is nice thing todo. For reading ops, it should be performed deferly. According to gRPC server-side stream semantics, currently it is always no-op except when user interrupts reading when data is no longer needed

@cthulhu-rider commented on GitHub (Dec 10, 2024): @roman-khimov i meant this in terms of stats. We _can_ submit real stats when stream is done and `Read()` returns an error regardless of closing. This is what i actually propose. I just noticed `Failure reason can be received via Close.` in docs, and this is not quire true. After `Read()` returns an error, `Close()` is no-op and returns the same overall, i agree that closer is nice thing todo. For reading ops, it should be performed `defer`ly. According to gRPC server-side stream semantics, currently it is always no-op except when user interrupts reading when data is no longer needed

sami commented

2025-12-28 18:07:35 +00:00

Author

Owner

@roman-khimov commented on GitHub (Dec 10, 2024):

We can do a lot of things, the question is should we? To me it works fine already wrt statistics. Users must Close() and that's where you get your overall statistics.

@roman-khimov commented on GitHub (Dec 10, 2024): We can do a lot of things, the question is _should_ we? To me it works fine already wrt statistics. Users must `Close()` and that's where you get your overall statistics.

sami commented

2025-12-28 18:07:35 +00:00

Author

Owner

@cthulhu-rider commented on GitHub (Dec 10, 2024):

Users must Close()

the imposition is excessive, the stream works differently. UX improvement is desirable imo. Being a user, i'd be grateful for the stats that came even if I forgot closer

@cthulhu-rider commented on GitHub (Dec 10, 2024): > Users must Close() the imposition is excessive, the stream works differently. UX improvement is desirable imo. Being a user, i'd be grateful for the stats that came even if I forgot closer

sami commented

2025-12-28 18:07:35 +00:00

Author

Owner

@roman-khimov commented on GitHub (Dec 10, 2024):

I doubt anyone cares much about streams that drive all of this. But people know https://pkg.go.dev/os#File, they know https://pkg.go.dev/io#ReadCloser, not calling Close() for os.File is an error, for io.ReadCloser the general expectation is the same.

@roman-khimov commented on GitHub (Dec 10, 2024): I doubt anyone cares much about streams that drive all of this. But people know https://pkg.go.dev/os#File, they know https://pkg.go.dev/io#ReadCloser, not calling `Close()` for `os.File` is an error, for `io.ReadCloser` the general expectation is the same.

sami commented

2025-12-28 18:07:35 +00:00

Author

Owner

@roman-khimov commented on GitHub (Dec 10, 2024):

Body in https://pkg.go.dev/net/http#Response is the same, btw. It is an error not to Close() it.

@roman-khimov commented on GitHub (Dec 10, 2024): Body in https://pkg.go.dev/net/http#Response is the same, btw. It is an error not to `Close()` it.

sami commented

2025-12-28 18:07:35 +00:00

Author

Owner

@cthulhu-rider commented on GitHub (Dec 10, 2024):

I doubt anyone cares much about streams that drive all of this. But people know https://pkg.go.dev/os#File, they know https://pkg.go.dev/io#ReadCloser, not calling Close() for os.File is an error, for io.ReadCloser the general expectation is the same.

interface itself states no such requirements. 'Specific implementations may document their own behavior' and that's it

op stats and resource release in different planes to me. Exactly like successful reading 1GB data from file and forgotten descriptor

@cthulhu-rider commented on GitHub (Dec 10, 2024): > I doubt anyone cares much about streams that drive all of this. But people know https://pkg.go.dev/os#File, they know https://pkg.go.dev/io#ReadCloser, not calling `Close()` for `os.File` is an error, for `io.ReadCloser` the general expectation is the same. interface itself states no such requirements. 'Specific implementations may document their own behavior' and that's it op stats and resource release in different planes to me. Exactly like successful reading 1GB data from file and forgotten descriptor