search

nats-io / nats.c

A C client for NATS
Apache License 2.0
390 stars 137 forks source link

Crash when using natsMsg_Destroy on a JetStream message #793

Closed thierryba closed 2 months ago

thierryba commented 2 months ago

Observed behavior

If one is deleting a message from jetstream asynchronously with natsMsg_Destroy, it will crash (https://github.com/nats-io/nats.c/pull/792 exhibits this in a test)

Expected behavior

it should not crash and I should be able to take a natsMsg and decide when I want to use natsMsg_Destroy on it. If I missed something, please correct me.

Server and client version

nats-server: v2.10.18 nats.c version 3.8.2 (the crash still happens with main, see test for it)

Host environment

MacOS 14, arm64

Steps to reproduce

see the draft PR for a test: https://github.com/nats-io/nats.c/pull/792

thierryba commented 2 months ago

I nailed it down to the _autoAckCB that actually destroys the message. That is a bit weird and defeats the purpose of having a callback, That callback should then never take a reference to the natsMsg unless we do manual ack.

kozlovic commented 2 months ago

I was pretty sure that this was documented somewhere in the doc (nats.h), but can't clearly see it. But yes, the library "disables" the destroy so that when the caller calls destroys in the callback, the library still has access to it to ack the message, then internally destroys it.

Remember, natsMsg do not have lock and no ref counting, so we are limited in what we can let the user do with a message when the library still needs access to it.

The library could have done without the message by collecting the required information, which in this case is the reply subject really. But that would have mean to make a copy of that string.

thierryba commented 2 months ago

hmm ok, got it. I've seen that manual ack also destroys the message. So I will "just" rely on that. Thx.

thierryba commented 2 months ago

you could have left it to the callback to get rid of the message.

kozlovic commented 2 months ago

you could have left it to the callback to get rid of the message.

But again, the library - with auto-ack - needs to ack after the callback returns. So if the message is destroyed, the reference to the reply subject would be invalid, hence my comment regarding the need to make a copy of that string, which currently we don't need because we "disable" the destroy of the message by the user (using an internal flag).