Expose MaxPendingBytes on the natsOptions #700

LaurensVergote · 2023-11-27T12:02:16Z

Similar as https://github.com/nats-io/nats.net/pull/844 from the nats.net library.
I've used the same defaults as the C# library

Couldn't get the testsuite to work locally so I hope the github pipeline will run them :)

kozlovic

First, thank you for the contribution. But I am afraid that this could break applications the way it is coded. Review my comments. You missed a step to regenerate the test/list.txt, this is why you were not able to run the tests (and CI did not either).
Finally, since this is added a new API (and unless @levb no longer plans on using the dev branch), this PR should have been made against the dev branch.

kozlovic · 2023-11-27T19:39:22Z

src/natsp.h

@@ -265,6 +265,7 @@ struct __natsOptions
    int64_t                 pingInterval;
    int                     maxPingsOut;
    int                     maxPendingMsgs;
+    long                    maxPendingBytes;


Make this an int64_t (although could arguably made an uint64_t, but in case we want to use negative value to mean something).

kozlovic · 2023-11-27T19:39:33Z

src/opts.c

@@ -883,6 +883,18 @@ natsOptions_SetMaxPendingMsgs(natsOptions *opts, int maxPending)
    return NATS_OK;
 }

+natsStatus
+natsOptions_SetMaxPendingBytes(natsOptions* opts, long maxPending)


kozlovic · 2023-11-27T19:41:39Z

src/sub.c

@@ -454,7 +450,7 @@ natsSub_create(natsSubscription **newSub, natsConnection *nc, const char *subj,
    sub->msgCb          = cb;
    sub->msgCbClosure   = cbClosure;
    sub->msgsLimit      = nc->opts->maxPendingMsgs;
-    sub->bytesLimit     = bytesLimit;
+    sub->bytesLimit     = nc->opts->maxPendingBytes;


So we are changing behavior here. If the user set the option of maxPendingMsgs, we would set the limit to that value * 1024 (by default). Now it would be the default value that you have set to 64MB. In other words, that could break applications.
I would suggest that we don't set a default then, and so, if <=0, we multiply the maxPendingMsgs by 1024, otherwise, we use the set value.

Checked for NATS_OPTS_DEFAULT_MAX_PENDING_BYTES equality instead of not setting a default.

kozlovic · 2023-11-27T19:41:50Z

test/test.c

@@ -2580,6 +2580,7 @@ test_natsOptions(void)
             && (opts->maxPingsOut == 2)
             && (opts->ioBufSize == 32 * 1024)
             && (opts->maxPendingMsgs == 65536)
+             && (opts->maxPendingBytes == 67108864)


Maybe not, see comment above.

Should be valid with reviewed implementation

Would not be if you agree with my earlier comment.

test/test.c

LaurensVergote · 2023-11-28T15:08:46Z

Not quite sure why the CI timed out on one of the new tests, had no issues locally running it

kozlovic · 2023-11-28T16:21:16Z

src/sub.c

@@ -454,7 +451,7 @@ natsSub_create(natsSubscription **newSub, natsConnection *nc, const char *subj,
    sub->msgCb          = cb;
    sub->msgCbClosure   = cbClosure;
    sub->msgsLimit      = nc->opts->maxPendingMsgs;
-    sub->bytesLimit     = bytesLimit;
+    sub->bytesLimit     = nc->opts->maxPendingBytes == NATS_OPTS_DEFAULT_MAX_PENDING_BYTES ? nc->opts->maxPendingMsgs * 1024 : nc->opts->maxPendingBytes;;


Not really. Because if the user happens to want to set the limit to 64MB, then the actual limit would be whatever maxPendingMsgs*1024 is. This is incorrect. So what I was suggesting is either we make it a breaking change (and since it is a new API it should only go in 3.8.0 anyway), or we initialize the value to -1, so that we can detect here that it was not user-set, and if that is the case, then use the "old" formula (pendingMsgs * 1024) or if set use the provided value. So there would not be a need for the new NATS_OPTS_DEFAULT_MAX_PENDING_BYTES constant.

kozlovic · 2023-11-28T16:21:43Z

test/test.c

@@ -2580,6 +2580,7 @@ test_natsOptions(void)
             && (opts->maxPingsOut == 2)
             && (opts->ioBufSize == 32 * 1024)
             && (opts->maxPendingMsgs == 65536)
+             && (opts->maxPendingBytes == 67108864)


Would not be if you agree with my earlier comment.

kozlovic · 2023-11-28T16:27:52Z

test/test.c

+    size_t data_len = 10;
+    const char msg[] = { 0,1,2,3,4,5,6,7,8,9 }; //10 bytes long message
+
+    for (int i = 0;


For code consistency. It came from a time where - I believe on Windows, with whatever compiler version we were using - it was not possible to declare variables there. We could relax that, especially in test.c, but I like to keep things consistent. It also help prevent variable shadowing bugs.

kozlovic · 2023-11-28T16:33:53Z

test/test.c

@@ -14345,7 +14350,7 @@ _asyncErrCb(natsConnection *nc, natsSubscription *sub, natsStatus err, void* clo
 }

 static void
-test_AsyncErrHandler(void)
+test_AsyncErrHandler_MaxPendingMsgs(void)


I believe the issue is in _asyncErrCb (I know it is not related to your change). Could you please make the following change?

diff --git a/test/test.c b/test/test.c index 2c164b7f..59f9824d 100644 --- a/test/test.c +++ b/test/test.c @@ -14344,7 +14344,7 @@ _asyncErrCb(natsConnection *nc, natsSubscription *sub, natsStatus err, void* clo arg->closed = true; arg->done = true; - natsCondition_Signal(arg->c); + natsCondition_Broadcast(arg->c); natsMutex_Unlock(arg->m); }

The issue is that the subscription's callback is waiting for closed to be set to true and waits on the condition variable. Since both the test and the callback wait on the async error callback notification, there is a chance that the signal leaves the callback blocked. Doing the broadcast should solve that.

kozlovic

Let's fix the for loop to not have the warning. But more importantly, we need @levb to confirm if we are still using the dev branch for new features. If so, this PR will need to change the base branch from main to dev. As of now though, I don't think that dev is up-to-date, so let's wait on @levb to confirm what we need to do here.

kozlovic · 2023-11-30T16:38:17Z

test/test.c

+    size_t data_len = 10;
+    const char msg[] = { 0,1,2,3,4,5,6,7,8,9 }; //10 bytes long message
+
+    for (int i = 0;


That produces warnings:

[ 98%] Building C object test/CMakeFiles/testsuite.dir/test.c.o /home/travis/build/nats-io/nats.c/test/test.c: In function ‘test_AsyncErrHandler_MaxPendingBytes’: /home/travis/build/nats-io/nats.c/test/test.c:14453:5: warning: statement with no effect [-Wunused-value] 14453 | for (i; | ^~~

Instead, do for (i=0; ....

kozlovic · 2023-11-30T16:38:46Z

test/test.c

+    natsMutex_Unlock(arg.m);
+
+    test("Cause an error by sending too many messages: ");
+    for (i;


Same comment than in the other test.

kozlovic

LGTM. Will let @levb have a look and merge if he is ok with it.

levb

LGTM

…ions (#700)

Laurens Vergote added 2 commits November 27, 2023 13:00

Expose MaxPendingBytes on the natsOptions

c6aff34

Fix tests and declare new method in header

5227867

kozlovic requested changes Nov 27, 2023

View reviewed changes

Fix comments

123bef3

kozlovic requested changes Nov 28, 2023

View reviewed changes

Preserve old behaviour exactly

276e42e

kozlovic requested changes Nov 30, 2023

View reviewed changes

levb and others added 5 commits January 16, 2024 05:07

Merge branch 'main' into MaxPendingBytes

94d670e

Merge branch 'main' into MaxPendingBytes

af1c698

Merge branch 'main' into MaxPendingBytes

eb93999

Fix Comments

9ea25b2

Merge branch 'main' into MaxPendingBytes

bff920c

kozlovic approved these changes Apr 5, 2024

View reviewed changes

levb approved these changes Apr 5, 2024

View reviewed changes

levb merged commit ff8528f into nats-io:main Apr 5, 2024
1 check passed

github-actions bot pushed a commit that referenced this pull request Apr 5, 2024

[skip ci] Update docs: ff8528f: Expose MaxPendingBytes on the natsOpt…

1028572

…ions (#700)

levb mentioned this pull request Apr 5, 2024

Auto-generated documentation update for main #742

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose MaxPendingBytes on the natsOptions #700

Expose MaxPendingBytes on the natsOptions #700

LaurensVergote commented Nov 27, 2023 •

edited

Loading

kozlovic left a comment

kozlovic Nov 27, 2023

LaurensVergote Nov 28, 2023

kozlovic Nov 27, 2023

LaurensVergote Nov 28, 2023

kozlovic Nov 27, 2023

LaurensVergote Nov 28, 2023

kozlovic Nov 27, 2023

LaurensVergote Nov 28, 2023

kozlovic Nov 28, 2023

LaurensVergote Nov 30, 2023

LaurensVergote commented Nov 28, 2023

kozlovic Nov 28, 2023

LaurensVergote Nov 30, 2023

kozlovic Nov 28, 2023

kozlovic Nov 28, 2023

kozlovic Nov 28, 2023

LaurensVergote Nov 30, 2023

kozlovic left a comment

kozlovic Nov 30, 2023

kozlovic Nov 30, 2023

LaurensVergote Apr 5, 2024

kozlovic left a comment

levb left a comment

Expose MaxPendingBytes on the natsOptions #700

Expose MaxPendingBytes on the natsOptions #700

Conversation

LaurensVergote commented Nov 27, 2023 • edited Loading

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaurensVergote commented Nov 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

levb left a comment

Choose a reason for hiding this comment

LaurensVergote commented Nov 27, 2023 •

edited

Loading