[CHANGED] refactored nats.c, prep for js_PullSubscribeAsync #778
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## lev-bench2 #778 +/- ##
==============================================
- Coverage 68.84% 68.80% -0.04%
==============================================
Files 39 49 +10
Lines 15238 15206 -32
Branches 3153 3135 -18
==============================================
- Hits 10491 10463 -28
- Misses 1680 1701 +21
+ Partials 3067 3042 -25 ☔ View full report in Codecov by Sentry. |
|
performance benchmark: https://github.com/nats-io/nats.c/actions/runs/10267478369/job/28408116810?pr=778 Step: compare benchmarks |
There was a problem hiding this comment.
Some changes.
Now, I need to look back at some comment I made in an earlier PR regarding the delivery lock situation, that we could modify something and reduce a lot the use of it. It was a moot point since you started refactoring that (and got rid of altogether), but now that we are back at having the two, I wonder if this would still be applicable.
src/dispatch.c
Outdated
| signal = true; | ||
| msg->next = NULL; | ||
| q->head = msg; | ||
| if (q->tail == NULL) |
There was a problem hiding this comment.
Would remove, since tail must be NULL if head is NULL. Also, q->tail = msg; should be done in all cases so would move out of the if/else.
src/dispatch.c
Outdated
| else | ||
| { | ||
| q->tail->next = msg; | ||
| q->tail = msg; |
|
|
||
| s = natsMutex_Create(&d->mu); | ||
| if (s != NATS_OK) | ||
| return s; |
There was a problem hiding this comment.
return NATS_UPDATE_ERR_STACK(s);
There was a problem hiding this comment.
(that being said, not sure what happens since this is a static inline function...)
There was a problem hiding this comment.
Should be the same since func will still be defined.
src/glib/glib_dispatch_pool.c
Outdated
| _destroyDispatcher(d); | ||
| natsLib_Release(); | ||
| } | ||
| return s; |
There was a problem hiding this comment.
return NATS_UPDATE_ERR_STACK(s);
src/glib/glib_dispatch_pool.c
Outdated
| pool->cap = cap; | ||
| } | ||
| } | ||
| return s; |
There was a problem hiding this comment.
return NATS_UPDATE_ERR_STACK(s);
|
|
||
| static inline void _cleanupOwnDispatcher(natsSubscription *sub) | ||
| { | ||
| nats_destroyQueuedMessages(&sub->ownDispatcher.queue); |
There was a problem hiding this comment.
Again, cleanup/destroy functions may be called as part of the creation of the object and you have to assume that the failure could happen at any point and therefore some fields may not have been initialized. So here ownDispatcher seem to be a struct, so it won't be NULL, but queue may, yet nats_destroyQueuedMessages does not seem to be checking for NULL there.
There was a problem hiding this comment.
Same, queue is not a pointer; a NULL-ed out struct, so the for loop bails.
| @@ -301,7 +310,7 @@ natsSub_deliverMsgs(void *arg) | |||
| } | |||
|
|
|||
| natsSub_Lock(sub); | |||
| onCompleteCB = sub->onCompleteCB; | |||
There was a problem hiding this comment.
That is an example of what I was talking about :-)
There was a problem hiding this comment.
I am not sure I am following :) I think this one is VERY low overhead since it's done once, so kind of does not matter that we re-lock here?
There was a problem hiding this comment.
Oh no, I was just saying that I was aligning the = ... with the line below, that was a comment about "tabulation", that's all.
There was a problem hiding this comment.
@kozlovic do you mind emailing me a copy of your VSCode settings.json so I can sync the auto-formatting?
src/sub.c
Outdated
| { | ||
| natsStatus s = NATS_OK; | ||
| if (sub->ownDispatcher.thread != NULL) | ||
| return NATS_ILLEGAL_STATE; // already running |
src/sub.c
Outdated
|
|
||
| sub->dispatcher = &sub->ownDispatcher; | ||
| s = natsThread_Create(&sub->ownDispatcher.thread, natsSub_deliverMsgs, (void *) sub); | ||
| return s; |
… js_PullSubscribeAsync (#778)
This is a replacement for #772 that had too many changes/re-bases and had grown too complicated for review.