The insert notification was done based on the creation of a node
during an insert, which was wrong since the node may have already
existed and still the subscription could be all new. For instance,
suppose that there is a subscription on "foo.bar".
We register an notification interest for "foo", which does not
notify, which is normal. Then we create a subscription on "foo".
During the insert, "foo" node already exists so notification would
not be sent, but it should.
Fixed also removed by having removeFromNode() returning a boolean
to indicate if the subscription was the last in that node.
However, it seems that we again check for interest in
chkForRemoveNotification(), so not sure if that is required.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
This is how it was up to v2.1.2 included (changed in v2.1.4 onward).
I added a benchmark that has 3 subscribers running and increase
the number of publishers: 1, 2, 5 and 10. This is the comparison
between the pre-PR and post-PR:
```
benchcmp old.txt new.txt
benchmark old ns/op new ns/op delta
Benchmark___BumpPubCount_1x3-16 396 385 -2.78%
Benchmark___BumpPubCount_2x3-16 495 406 -17.98%
Benchmark___BumpPubCount_5x3-16 542 395 -27.12%
Benchmark__BumpPubCount_10x3-16 549 515 -6.19%
benchmark old MB/s new MB/s speedup
Benchmark___BumpPubCount_1x3-16 717.27 737.54 1.03x
Benchmark___BumpPubCount_2x3-16 574.31 699.02 1.22x
Benchmark___BumpPubCount_5x3-16 524.35 718.80 1.37x
Benchmark__BumpPubCount_10x3-16 517.26 551.53 1.07x
```
It is inline with what the user reported, seeing a 20% drop in performance
when going from 1 publisher to 2. But, as we can see, the difference
between go channel and cond variable reduces with the increased number
of publishers after a certain number.
I am not sure of the performance impact on other situations, so this
PR is more of a proposal than a fix.
Resolves#1786
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Only the user (from username/password connection method) was reported
in this monitoring endpoint. Will now report proper nkey, public key,
etc..
Resolves#1799
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Imports in JWT where extended to contain a new filed LocalSubject.
This Change pulls the new JWT library version in.
It was needed as prefix did not exist in the JWT library and the
original field could not be used. The field To has been deprecated.
When LocalSubject is set, service imports can be configured the same way
they are in config. Meaning, no reversal due to the type.
This change also ensures that wildcard references in transforms are only
set in To/LocalSubject. Before, for services, $1 would have to be set in Subject.
Signed-off-by: Matthias Hanel <mh@synadia.com>
PR #1412 had a fix for races during implicit GW reconnection.
However, the fix was a bit too simplistic in that it was checking
only if there was any inbound gateway to decide to try to reconnect
an implicit disconnected GW. We need to check the name, not only
presence of inbound GW connections.
Related to #1412
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
The server that solicits a LeafNode connection does not send an
INFO, so the accepting side had no way to know if the remote
supports headers or not. The solicit side will now send the headers
support capability in the CONNECT protocol so that the receiving
side can mark the inbound connection with headers support based
on that and its own support for headers.
Resolves#1781
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Based on timing, it is possible that the first error is about
connection refused as opposed to "Loop detected". So use a dedicated
logger to notify only when expected error is found.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Presence of TLS config in any remote gateway or leafnode would
cause the config reload to fail (because TLS config internal
content may change which fails the DeepEqual check).
This PR excludes the TLS configs in such case to check for
changes in gateways and leafnodes.
Although GW and LN config reload is technically supported, this
PR updates the internal remotes' TLS configuration so that
changes/updates to TLS certificates would take effect after
a configuration reload.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
- Keep track of published topic to avoid conversion in some case
- Rework tracing of some MQTT protocols
- Rework topic-to-subject conversion
- Update some tests
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
The test TestMQTTPersistedSession() flapped once on GA. It turns
out that when the server was sending CONNACK the test was immediately
using a NATS publisher to send a message that was not received by
the MQTT subscription for the recovered session.
Sending the CONNACK before restoring subscriptions allowed for a
window where a different connection could publish and messages
would be missed. It is technically ok, I think, and test could
have been easily fixed to ensure that we don't NATS publish before
the session is fully restored.
However, I have changed the order to first restore subscriptions
then send the CONNACK. The way locking happens with MQTT subscriptions
we are sure that the CONNACK will be sent first because even if
there are inflight messages, the MQTT callbacks will have to wait
for the session lock under which the subscriptions are restored
and the CONNACK sent.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Had a deadlock with new preconditions. We need to hold lock across Store() call but that call could call into storeUpdate() such that we may need to acquire the lock. We can enter this callback from the storage layer itself and the lock would not be held so added an atomic.
Signed-off-by: Derek Collison <derek@nats.io>
