refactor: Replace subscription events publisher #2686
Conversation
nasdf
added
the
refactor
nasdf
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2686 +/- ##
===========================================
- Coverage 78.16% 78.08% -0.08%
===========================================
Files 308 308
Lines 23081 23077 -4
===========================================
- Hits 18040 18019 -21
- Misses 3675 3687 +12
- Partials 1366 1371 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -265,9 +265,9 @@ type RequestResult struct { | |||
| // GQL contains the immediate results of the GQL request. | |||
| GQL GQLResult | |||
|
|
|||
| // Pub contains a pointer to an event stream which channels any subscription results | |||
| // if the request was a GQL subscription. | |||
| Pub *events.Publisher[events.Update] | |||
There was a problem hiding this comment.
discussion: I think you are removing functionality here - IIRC the events package stuff allows for multiple concurrent readers of the stream. The simple Go chan that you are replacing it lacks this functionality and will mean any concurrent readers will be competing with each other for each item.
The CLI/http clients are unaffected, but the embedded Go client is.
There was a problem hiding this comment.
It makes more sense to me that we assume one reader per request by default. The consumer of the channel then has the option to decide if they want to create an events.Publisher and publish the channel results to it.
| return nil, client.NewErrUnexpectedType[request.ObjectSubscription]("SubscriptionSelection", selections) | ||
| } | ||
| // unsubscribing from this publisher will cause a race condition | ||
| // https://github.com/sourcenetwork/defradb/issues/2687 |
There was a problem hiding this comment.
question: Is this a new issue, or was the old code affected to?
And where is the race condition? Is it a test artefact, or production code?
There was a problem hiding this comment.
I believe this is an old issue. The previous code never called Unsubscribe. The race happens between the db.Close and the pub.Unsubscribe so it's possible to happen in production, but likely would cause no issues.
| select { | ||
| case val := <-pub.Event(): | ||
| evt = val | ||
| case <-ctx.Done(): |
There was a problem hiding this comment.
todo: By not looping through the pub.Event() with a direct for loop, I think you may have removed the handling of a timeout - it looks like in the event of a timeout evt will be default/empty and bad things might happen later in this func.
Can you please confirm the behaviour, and make any necessary adjustments please?
(note: IIRC testing of subscriptions is not as comprehensive as most of our other queries, and it uses an old framework)
There was a problem hiding this comment.
Good catch! I've updated it to handle the channel closing.
Relevant issue(s)
Resolves #2685
Description
This PR replaces subscription event publishers with a simple go channel. This is a pre-requisite to a follow up events package refactor.
Tasks
How has this been tested?
make testSpecify the platform(s) on which this was tested: