mclock: Initial implementation #1

Merged
dstepanov-yadro merged 10 commits from dstepanov-yadro/frostfs-qos:feat/mclock into master 2025-01-28 11:27:24 +00:00

Initial mClock scheduler implementation.

Initial mClock scheduler implementation.
dstepanov-yadro added 1 commit 2025-01-20 14:11:18 +00:00
[#1] mclock: Initial implementation
Some checks failed
Tests and linters / Lint (pull_request) Failing after 16s
Tests and linters / gopls check (pull_request) Failing after 13s
Tests and linters / Staticcheck (pull_request) Failing after 17s
Tests and linters / Run gofumpt (pull_request) Failing after 17s
DCO action / DCO (pull_request) Successful in 30s
Tests and linters / Tests (pull_request) Successful in 32s
Vulncheck / Vulncheck (pull_request) Successful in 29s
Tests and linters / Tests with -race (pull_request) Failing after 35s
Pre-commit hooks / Pre-commit (pull_request) Failing after 48s
7444583f12
Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
dstepanov-yadro added 1 commit 2025-01-20 14:15:26 +00:00
[#1] Makefile: Create directory with parents
Some checks failed
Tests and linters / Tests (pull_request) Successful in 24s
DCO action / DCO (pull_request) Successful in 28s
Tests and linters / Run gofumpt (pull_request) Successful in 26s
Tests and linters / Tests with -race (pull_request) Failing after 38s
Vulncheck / Vulncheck (pull_request) Successful in 37s
Tests and linters / Staticcheck (pull_request) Successful in 41s
Pre-commit hooks / Pre-commit (pull_request) Failing after 49s
Tests and linters / Lint (pull_request) Successful in 1m4s
Tests and linters / gopls check (pull_request) Successful in 1m44s
c65068d4e2
Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
dstepanov-yadro force-pushed feat/mclock from c65068d4e2 to 95ccf46c64 2025-01-20 14:23:22 +00:00 Compare
dstepanov-yadro added 1 commit 2025-01-20 14:26:49 +00:00
[#1] pre-commit: Use local golangci-lint hook
All checks were successful
DCO action / DCO (pull_request) Successful in 28s
Tests and linters / Run gofumpt (pull_request) Successful in 27s
Vulncheck / Vulncheck (pull_request) Successful in 31s
Tests and linters / Staticcheck (pull_request) Successful in 40s
Tests and linters / Lint (pull_request) Successful in 1m4s
Pre-commit hooks / Pre-commit (pull_request) Successful in 1m12s
Tests and linters / Tests (pull_request) Successful in 1m18s
Tests and linters / Tests with -race (pull_request) Successful in 1m19s
Tests and linters / gopls check (pull_request) Successful in 1m24s
c6789eea71
Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
requested reviews from storage-core-committers, storage-core-developers 2025-01-21 06:02:00 +00:00
acid-ant reviewed 2025-01-21 09:34:54 +00:00
@ -0,0 +147,4 @@
}
}
// Close clocses MClock scheduler.
Member

clocses -> closes

clocses -> closes
Author
Member

done

done
dstepanov-yadro force-pushed feat/mclock from c6789eea71 to 2ef8e5508f 2025-01-21 09:42:16 +00:00 Compare
aarifullin reviewed 2025-01-22 14:05:50 +00:00
@ -0,0 +62,4 @@
Shares float64
}
// MClock is mClock scheduling algorithm realization.
Member

realization -> implementation?
Realize is rather about plans in general :)

`realization` -> `implementation`? Realize is rather about plans in general :)
Author
Member

fixed

fixed
aarifullin marked this conversation as resolved
dstepanov-yadro force-pushed feat/mclock from 2ef8e5508f to 170388db8d 2025-01-22 14:20:22 +00:00 Compare
aarifullin reviewed 2025-01-22 15:26:59 +00:00
@ -0,0 +215,4 @@
canceled: make(chan struct{}),
}
if tagInfo.Reservation != nil {
r.reservation = max(prev.reservation + 1.0 / *tagInfo.Reservation, now)
Member

I suppose we should add some value validation for tags as we expect that *tagInfo.Reservation != 0

I suppose we should add some value validation for tags as we expect that `*tagInfo.Reservation != 0`
Author
Member

Added validation and unit tests

Added validation and unit tests
aarifullin marked this conversation as resolved
aarifullin reviewed 2025-01-22 15:40:34 +00:00
@ -0,0 +514,4 @@
}
func newSystemClock() *systemClock {
c := &systemClock{
Member

schedule is never initialized - way to panic?

`schedule` is never initialized - way to panic?
Author
Member

fixed

fixed
aarifullin marked this conversation as resolved
dstepanov-yadro force-pushed feat/mclock from 170388db8d to 0edfa036a5 2025-01-22 16:09:43 +00:00 Compare
dstepanov-yadro force-pushed feat/mclock from 0edfa036a5 to 3d0219d012 2025-01-23 06:47:22 +00:00 Compare
aarifullin reviewed 2025-01-23 10:16:45 +00:00
@ -0,0 +585,4 @@
f = nil
continue
}
t.Reset(time.Duration((s.ts - now) * 1e9))
Member

https://pkg.go.dev/time#Timer.Reset

We use go 1.22 here. So, it seems we need to drain the timer before reset

if !t.Stop() {
    <-t.C
}

Either Reset may not reset

https://pkg.go.dev/time#Timer.Reset We use `go 1.22` here. So, it seems we need to drain the timer before reset ```go if !t.Stop() { <-t.C } ``` Either `Reset` may not reset
Author
Member

Thx, fixed!

Thx, fixed!
aarifullin marked this conversation as resolved
dstepanov-yadro force-pushed feat/mclock from 3d0219d012 to 180e62c296 2025-01-23 10:26:16 +00:00 Compare
a-savchuk reviewed 2025-01-23 11:21:24 +00:00
@ -0,0 +415,4 @@
// waitingCount is for using in tests only.
//
// nolint: unused
Member

This comment is outdated - you already use this function in the tests. Please, remove it

This comment is outdated - you already use this function in the tests. Please, remove it
Author
Member

golangci-lint doesn't includes tests in check, so this comment is required

golangci-lint doesn't includes tests in check, so this comment is required
a-savchuk marked this conversation as resolved
@ -0,0 +536,4 @@
type systemClock struct {
since time.Time
schedule chan scheduleInfo
wg sync.WaitGroup
Member

What do you think about using a channel instead of a WaitGroup? I think it'll be enough for your current purposes

What do you think about using a channel instead of a `WaitGroup`? I think it'll be enough for your current purposes
Author
Member

I prefer wait group)

I prefer wait group)
a-savchuk marked this conversation as resolved
@ -0,0 +19,4 @@
defaultLimit float64 = 100_000
shortReservation float64 = 1
medReservation float64 = 100
largeReservation float64 = 100_00
Member

I think 10_000 would look better

I think `10_000` would look better
Author
Member

Fixed

Fixed
a-savchuk marked this conversation as resolved
@ -0,0 +66,4 @@
if limit != nil {
limitStr = strconv.FormatFloat(*limit, 'f', 1, 64)
}
b.Run(fmt.Sprintf("mclock, %s limit, %s reservation, %d parallelism, %d tags", limitStr, resStr, parallelism, tags), func(b *testing.B) {
Member

Could we use benchstat syntax for logging benchmark parameters?

TrueCloudLab/frostfs-node#1472
https://pkg.go.dev/golang.org/x/perf/benchproc/syntax

Could we use benchstat syntax for logging benchmark parameters? https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/1472 https://pkg.go.dev/golang.org/x/perf/benchproc/syntax
a-savchuk reviewed 2025-01-23 13:26:36 +00:00
@ -0,0 +289,4 @@
defer q.mtx.Unlock()
}
if q.inProgress >= q.runLimit {
Member

When can inProgress be greater than runLimit?

When can `inProgress` be greater than `runLimit`?
Author
Member

These values can be equal. Greater check is just foolproof.

These values can be equal. Greater check is just foolproof.
a-savchuk marked this conversation as resolved
@ -0,0 +394,4 @@
heap.Remove(q.readyQueue, r.readyIdx)
}
if r.reservationIdx != invalidIndex {
heap.Remove(q.reservationQueue, r.readyIdx)
Member

You used readyIdx instead of reservationIdx. Typo?

You used `readyIdx` instead of `reservationIdx`. Typo?
Author
Member

Thx, fixed!

Thx, fixed!
a-savchuk marked this conversation as resolved
dstepanov-yadro force-pushed feat/mclock from 180e62c296 to 62d5c81455 2025-01-23 14:51:11 +00:00 Compare
aarifullin approved these changes 2025-01-23 15:04:19 +00:00
Dismissed
aarifullin left a comment
Member

Excellent idea 👍

Excellent idea 👍
achuprov reviewed 2025-01-24 08:35:23 +00:00
@ -0,0 +313,4 @@
nextTs = q.limitQueue.items[0].ts()
}
if q.lastSchedule < now && q.lastSchedule > nextTs {
Member

What does this condition mean?

What does this condition mean?
Author
Member

Good question! It was an error.
It must be right so: if q.lastSchedule > nextTs
This means that nextTs is closer than already scheduled, so we must overwrite it.

Good question! It was an error. It must be right so: `if q.lastSchedule > nextTs` This means that nextTs is closer than already scheduled, so we must overwrite it.
achuprov marked this conversation as resolved
dstepanov-yadro added 2 commits 2025-01-24 09:18:44 +00:00
Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
[#1] mclock: Fix time based scheduling
All checks were successful
Tests and linters / Run gofumpt (pull_request) Successful in 32s
DCO action / DCO (pull_request) Successful in 48s
Vulncheck / Vulncheck (pull_request) Successful in 46s
Tests and linters / Staticcheck (pull_request) Successful in 50s
Pre-commit hooks / Pre-commit (pull_request) Successful in 1m14s
Tests and linters / Tests (pull_request) Successful in 1m24s
Tests and linters / Tests with -race (pull_request) Successful in 1m24s
Tests and linters / Lint (pull_request) Successful in 1m27s
Tests and linters / gopls check (pull_request) Successful in 1m53s
020829ced1
Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
dstepanov-yadro force-pushed feat/mclock from 020829ced1 to 90052efd34 2025-01-24 09:42:38 +00:00 Compare
achuprov approved these changes 2025-01-24 10:24:43 +00:00
dstepanov-yadro force-pushed feat/mclock from 90052efd34 to 095b24f865 2025-01-24 11:02:27 +00:00 Compare
aarifullin approved these changes 2025-01-24 14:01:36 +00:00
fyrchik reviewed 2025-01-27 10:09:11 +00:00
@ -0,0 +55,4 @@
}
// Release is the type of function that should be called after the request is completed.
type Release func()
Owner

ReleaseFunc?

`ReleaseFunc`?
Author
Member

Done

Done
fyrchik marked this conversation as resolved
@ -0,0 +106,4 @@
tagInfo: tagInfo,
reservationQueue: &queue{
items: make([]queueItem, 0),
Owner

make([]queueItem, 0) looks like an anti-pattern to me, why don't we use nil and omit this line?

`make([]queueItem, 0)` looks like an anti-pattern to me, why don't we use `nil` and omit this line?
Author
Member

Done

Done
fyrchik marked this conversation as resolved
@ -0,0 +176,4 @@
return ErrInvalidRunLimit
}
for _, v := range tagInfo {
if v.Limit != nil && *v.Limit <= float64(0) {
Owner

Hm, is float64 obligatory here?

Hm, is `float64` obligatory here?
Owner

Also, is there any reason we use floats instead of ints?

Also, is there any reason we use floats instead of ints?
Author
Member

Algorithm uses division, and the weights can be arbitrary. That's why I chose float64.

Algorithm uses division, and the weights can be arbitrary. That's why I chose float64.
Member

I thought about it too

Algorithm uses division

Your implementation uses seconds (float64 in Go) as a time unit. I think nanoseconds (int64 in Go) could be used too with some changes in the algorithm

For example, take the following formula

R_i^r = \max\left(R_i^{r-1} + \frac{1}{r_i},\ \mathrm{now}()\right)

Let's add units for each value

R_i^r\ \mathrm{[s]}= \max\left(R_i^{r-1}\ \mathrm{[s]} + \frac{1}{r_i\ \mathrm{[IOPS]}},\ \mathrm{now}()\ \mathrm{[s]}\right)

As I understand, 1\ \mathrm{IOPS} = 1\ \mathrm{s}^{-1}, so

r_i = 1\ \mathrm{IOPS} = 1\ \mathrm{s}^{-1} = 1 \cdot 10^9\ \mathrm{ns}^{-1}
R_i^r\ \mathrm{[s]}= \max\left(R_i^{r-1}\ \mathrm{[s]} + \frac{1}{r_i\ \mathrm{[s^{-1}]}},\ \mathrm{now}()\ \mathrm{[s]}\right)

Then rewrite all formulas

R_i^r\ \mathrm{[ns]}= \max\left(R_i^{r-1}\ \mathrm{[ns]} + \frac{10^9}{r_i\ \mathrm{[ns^{-1}]}},\ \mathrm{now}()\ \mathrm{[ns]}\right)
L_i^r\ \mathrm{[ns]}= \max\left(L_i^{r-1}\ \mathrm{[ns]} + \frac{10^9}{l_i\ \mathrm{[ns^{-1}]}},\ \mathrm{now}()\ \mathrm{[ns]}\right)
P_i^r\ \mathrm{[ns]}= \max\left(P_i^{r-1}\ \mathrm{[ns]} + \frac{10^9\ \mathrm{[ns]}}{w_i},\ \mathrm{now}()\ \mathrm{[ns]}\right)

What do you think?

I thought about it too > Algorithm uses division Your implementation uses _seconds_ (float64 in Go) as a time unit. I think _nanoseconds_ (int64 in Go) could be used too with some changes in the algorithm For example, take the following formula ```math R_i^r = \max\left(R_i^{r-1} + \frac{1}{r_i},\ \mathrm{now}()\right) ``` Let's add units for each value ```math R_i^r\ \mathrm{[s]}= \max\left(R_i^{r-1}\ \mathrm{[s]} + \frac{1}{r_i\ \mathrm{[IOPS]}},\ \mathrm{now}()\ \mathrm{[s]}\right) ``` As I understand, $1\ \mathrm{IOPS} = 1\ \mathrm{s}^{-1}$, so ```math r_i = 1\ \mathrm{IOPS} = 1\ \mathrm{s}^{-1} = 1 \cdot 10^9\ \mathrm{ns}^{-1} ``` ```math R_i^r\ \mathrm{[s]}= \max\left(R_i^{r-1}\ \mathrm{[s]} + \frac{1}{r_i\ \mathrm{[s^{-1}]}},\ \mathrm{now}()\ \mathrm{[s]}\right) ``` Then rewrite all formulas ```math R_i^r\ \mathrm{[ns]}= \max\left(R_i^{r-1}\ \mathrm{[ns]} + \frac{10^9}{r_i\ \mathrm{[ns^{-1}]}},\ \mathrm{now}()\ \mathrm{[ns]}\right) ``` ```math L_i^r\ \mathrm{[ns]}= \max\left(L_i^{r-1}\ \mathrm{[ns]} + \frac{10^9}{l_i\ \mathrm{[ns^{-1}]}},\ \mathrm{now}()\ \mathrm{[ns]}\right) ``` ```math P_i^r\ \mathrm{[ns]}= \max\left(P_i^{r-1}\ \mathrm{[ns]} + \frac{10^9\ \mathrm{[ns]}}{w_i},\ \mathrm{now}()\ \mathrm{[ns]}\right) ``` What do you think?
Author
Member

As for me, using float64 shouldn't lead to any problems, and I personally find it more comfortable working with seconds than with nanoseconds.
But also I changed units for idleTimeout and names of TagInfo fields.

As for me, using float64 shouldn't lead to any problems, and I personally find it more comfortable working with seconds than with nanoseconds. But also I changed units for `idleTimeout` and names of `TagInfo` fields.
@ -0,0 +182,4 @@
if v.Reservation != nil && *v.Reservation <= float64(0) {
return ErrInvalidTagInfo
}
if v.Shares <= float64(0) {
Owner

NaN will not trigger this condition, though it seems like it should.

`NaN` will not trigger this condition, though it seems like it should.
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -0,0 +428,4 @@
return q.sharesQueue.Len()
}
func assertIndexInvalid(r *request) {
Owner

We assert that the index is valid, no?

We _assert_ that the index is _valid_, no?
Author
Member

We assert that all request's indexes are invalid.

We assert that all request's indexes are invalid.
Owner

What is the goal of such operation?

What is the goal of such operation?
Author
Member

Just to check that request removed from all queues.

Just to check that request removed from all queues.
fyrchik marked this conversation as resolved
@ -0,0 +491,4 @@
i.r.reservationIdx = idx
}
var _ queueItem = &limitMQueueItem{}
Owner

Have you considered splitting code between multiple files? (queue-related, request-related, clock-related etc.)

Have you considered splitting code between multiple files? (queue-related, request-related, clock-related etc.)
Author
Member

Done

Done
fyrchik marked this conversation as resolved
dstepanov-yadro force-pushed feat/mclock from 095b24f865 to cb2263035a 2025-01-27 10:46:07 +00:00 Compare
fyrchik reviewed 2025-01-27 10:52:33 +00:00
@ -0,0 +396,4 @@
// waitingCount is for using in tests only.
//
// nolint: unused
Owner

Still need this?

Still need this?
Author
Member

Yes: waitingCount is used by tests only, but linter excludes test files.

Yes: `waitingCount` is used by tests only, but linter excludes test files.
Owner

Can we implement it in test file then?

Can we implement it in test file then?
Author
Member

Ok, done

Ok, done
fyrchik marked this conversation as resolved
dstepanov-yadro force-pushed feat/mclock from cb2263035a to b9abb25e2d 2025-01-27 13:36:50 +00:00 Compare
dstepanov-yadro added 1 commit 2025-01-27 13:47:05 +00:00
[#1] mclock: Use time.Duration for idle timeout
All checks were successful
DCO action / DCO (pull_request) Successful in 28s
Tests and linters / Run gofumpt (pull_request) Successful in 24s
Vulncheck / Vulncheck (pull_request) Successful in 35s
Tests and linters / Staticcheck (pull_request) Successful in 45s
Tests and linters / Lint (pull_request) Successful in 1m3s
Pre-commit hooks / Pre-commit (pull_request) Successful in 1m10s
Tests and linters / Tests (pull_request) Successful in 1m15s
Tests and linters / Tests with -race (pull_request) Successful in 1m17s
Tests and linters / gopls check (pull_request) Successful in 1m19s
7ca4c76093
Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
dstepanov-yadro force-pushed feat/mclock from 7ca4c76093 to 20a882fc37 2025-01-27 13:52:33 +00:00 Compare
dstepanov-yadro force-pushed feat/mclock from 20a882fc37 to 2e388f14ca 2025-01-28 06:51:14 +00:00 Compare
fyrchik reviewed 2025-01-28 07:27:54 +00:00
@ -0,0 +116,4 @@
// request with the tag is scheduled for execution,
// context ctx is canceled or the scheduler is closed.
// If the method call returned non-nil ReleaseFunc,
// then it should be called after the request is completed.
Owner

Probably s/should/MUST/?

Checking my understanding: if it is not called, the space is not released and the queue is eventually saturated.
However, Close() will release all requests, without surprises.
Right?

Probably `s/should/MUST/`? Checking my understanding: if it is not called, the space is not released and the queue is eventually saturated. However, `Close()` will release all requests, without surprises. Right?
Author
Member

If it is not called, the count of in progress requests will not decreased and the queue is eventually saturated.
Close() will release all requests, without surprises.

If it is not called, the count of in progress requests will not decreased and the queue is eventually saturated. `Close()` will release all requests, without surprises.
fyrchik marked this conversation as resolved
@ -0,0 +247,4 @@
}
minShare := q.sharesQueue.items[0].ts()
for _, item := range q.limitQueue.items { // limitQueue has all requests and sharesQueue may be fixed
limitItem := (item).(*limitMQueueItem)
Owner

Why do you have brackets around item?

Why do you have brackets around `item`?
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -0,0 +262,4 @@
}
func (q *MClock) scheduleRequest(lockTaken bool) {
if !lockTaken {
Owner

IMO making 2 separate functions would be more readable (i.e. schedule() { lock(); defer unlock(); scheduleUnsafe() } and scheduleUnsafe()), because it is not obvious what this bool value means without looking at the function definition.

IMO making 2 separate functions would be more readable (i.e. `schedule() { lock(); defer unlock(); scheduleUnsafe() }` and `scheduleUnsafe()`), because it is not obvious what this bool value means without looking at the function definition.
Author
Member

Done

Done
fyrchik marked this conversation as resolved
@ -0,0 +297,4 @@
if q.timeBasedScheduleTs > nextTs {
q.clock.runAt(nextTs, func() {
q.scheduleRequest(false)
Owner

There is a possible call-chain runAt() -> scheduleRequest() -> setNextScheduleTimer() -> runAt(), where the last runAt call blocks on a non-buffered schedule channel.

Why does this not happen? Can we make it more obvious?

There is a possible call-chain `runAt()` -> `scheduleRequest()` -> `setNextScheduleTimer()` -> `runAt()`, where the last `runAt` call blocks on a non-buffered `schedule` channel. Why does this not happen? Can we make it more obvious?
Author
Member

Thank you very much! This is really a bug!

scheduleRequest(lock taken)                                              ||
                  ||                                                     ||
                  ||                              time fired, scheduleRequest called
                  ||                                                     ||                  
                  ||                                                     ||
runAt() called, push to channel                                          ||
                  ||                                                     ||
                              deadlock             

Fixed:

  1. systemClock's timer created stopped
  2. scheduleRequest called only when timer is fired inside systemClock
  3. runAt has default branch, to if timer fired and other scheduleRequest called, we just skip push to schedule channel, because another runAt call will be performed by scheduleRequest
Thank you very much! This is really a bug! ``` scheduleRequest(lock taken) || || || || time fired, scheduleRequest called || || || || runAt() called, push to channel || || || deadlock ``` Fixed: 1. `systemClock`'s timer created stopped 2. `scheduleRequest` called only when timer is fired inside `systemClock` 2. `runAt` has `default` branch, to if timer fired and other `scheduleRequest` called, we just skip push to `schedule` channel, because another `runAt` call will be performed by `scheduleRequest`
@ -0,0 +309,4 @@
heap.Push(q.readyQueue, &readyMQueueItem{r: ready.r})
}
for q.inProgress < q.runLimit && q.readyQueue.Len() > 0 {
Owner

Just a comment, not expecting any changes in this PR:

This loop is run with under q.mtx lock, so arriving requests are blocked by this processing routine.
Not saying that this is a problem, but it could become in theory (latency spikes or slow degradation).
We have limited queue size, though. And new requests can be dropped, so the damage is resricted.

Just a comment, not expecting any changes in this PR: This loop is run with under `q.mtx` lock, so arriving requests are blocked by this processing routine. Not saying that this is a problem, but it could become in theory (latency spikes or slow degradation). We have limited queue size, though. And new requests can be dropped, so the damage is resricted.
@ -0,0 +328,4 @@
ri := i.(*reservationMQueueItem)
if ri.r.tag == next.r.tag && ri.r.reservation > next.r.reservation {
ri.r.reservation -= 1.0 / *tagInfo.ReservedIOPS
updated = true
Owner

Have you considered using heap.Fix(q.reservationQueue, i) here instead of heap.Init() after the loop?

Have you considered using `heap.Fix(q.reservationQueue, i)` here instead of `heap.Init()` after the loop?
Author
Member

I've been thinking about it. But iterating through the elements and changing the queue at the same time can be difficult to understand.

I've been thinking about it. But iterating through the elements and changing the queue at the same time can be difficult to understand.
Owner

Oh, yes, missed that it can be done multiple times.

Oh, yes, missed that it can be done multiple times.
fyrchik marked this conversation as resolved
@ -0,0 +397,4 @@
func assertIndexInvalid(r *request) {
if r.limitIdx != invalidIndex {
panic("limitIdx is not -1")
Owner

I like the practice of writing panic. You have if cond { panic(str) }, how about introducing assert(cond, str, ...)?
... is for avoiding concatenating strings unless assertion fails.

I like the practice of writing `panic`. You have `if cond { panic(str) }`, how about introducing `assert(cond, str, ...)`? `...` is for avoiding concatenating strings unless assertion fails.
Author
Member

Done

Done
fyrchik marked this conversation as resolved
dstepanov-yadro added 1 commit 2025-01-28 08:06:49 +00:00
[#1] mclock: Fix possible deadlock
All checks were successful
DCO action / DCO (pull_request) Successful in 33s
Tests and linters / Run gofumpt (pull_request) Successful in 32s
Vulncheck / Vulncheck (pull_request) Successful in 42s
Tests and linters / Staticcheck (pull_request) Successful in 49s
Tests and linters / Lint (pull_request) Successful in 1m6s
Pre-commit hooks / Pre-commit (pull_request) Successful in 1m11s
Tests and linters / Tests with -race (pull_request) Successful in 1m22s
Tests and linters / Tests (pull_request) Successful in 1m28s
Tests and linters / gopls check (pull_request) Successful in 1m24s
5a774e7b26
There is a possible call-chain `scheduleRequest()` -> `runAt()` -> `scheduleRequest()`,
so second `scheduleRequest()` may be locked on mutext held by first `scheduleRequest()`.

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
dstepanov-yadro force-pushed feat/mclock from 5a774e7b26 to f4d8ebf13d 2025-01-28 08:36:37 +00:00 Compare
fyrchik approved these changes 2025-01-28 11:13:14 +00:00
dstepanov-yadro merged commit f4d8ebf13d into master 2025-01-28 11:27:24 +00:00
dstepanov-yadro referenced this pull request from a commit 2025-01-28 11:27:26 +00:00
Sign in to join this conversation.
No description provided.