subsys/bluetooth/host/conn.c: conn->ref is not 0 after disconnected

[zfviwx]

Describe the bug
bt_conn_create_le() returns NULL the 2nd time it is called.

To Reproduce
Steps to reproduce the behavior:

1. Call bt_conn_create_le() and wait till connected callback is called
2. Call bt_conn_disconnect() and wait till disconnected callback is called
3. Call bt_conn_create_le() again with the same params

Expected behavior
bt_conn_create_le() returns pointer to the connection

Impact
Cannot reconnect after disconnected

Environment (please complete the following information):

• OS: Linux
• Toolchain Zephyr SDK
• Commit SHA or Version used 4f9b9a4

Additional context
According to the comments in file conn.c:

Line 2229: The last reference of the stack is released after the disconnected callback. (It is not true)

Line 1695: The last ref will be dropped during cleanup (It is not true)

Line 338: Release the reference we took for the very first state transition. (It is true. conn->ref is initialized to 1 at line 426. So the ref it increases for the first state transition is from 1 to 2, so after this unref conn->ref is 1). To make it able to be reconnected again, conn->ref needs to be 0 at this point.

[aescolar]

CC @joerchan

[joerchan]

@zfviwx I suspect the last reference is held by the application.
If you see the documentation for bt_conn_create_le:

 *  Returns a new reference that the the caller is responsible for managing.

Which means that the application needs to call bt_conn_unref(conn); once it is finished using the connection object.
In the central_hr sample this is done here
https://github.com/zephyrproject-rtos/zephyr/blob/master/samples/bluetooth/central_hr/src/main.c#L210

[zfviwx]

It does. It refs in connected callback and unrefs in the disconnected callback, exactly as what it does in Zephyr samples. And I stepped through the code and am sure the application unrefed it.

[jhedberg]

@zfviwx the return value of bt_conn_create_le() is a new reference as well, so if you're not unrefing it that's a bug in the application. Note that the documentation talks about dropping the last references of the stack. That doesn't mean that the application (or any other module) can't still hold their own references as well.

[joerchan]

It does. It refs in connected callback and unrefs in the disconnected callback, exactly as what it does in Zephyr samples. And I stepped through the code and am sure the application unrefed it.

No the zephyr central samples does not ref it in the connected callback.
The zephyr central samples has a ref from where it calls bt_conn_create_le until it is unref`ed in disconnected callback.

If you don't store the value returned from bt_conn_create_le in that way, then you have to unref the return from bt_conn_create_le.

[zfviwx]

@joerchan What is the logic that ref and unref don't come in pair? If the application doesn't ref it explicitly, why should it unref it? And it is not mentioned in the header.

[jhedberg]

@zfviwx we would risk race conditions if reference-counted pointers returned by functions don't give you a new reference. Let's say the caller has to do ref() itself, however before it has the chance to do that a context switch happens back to a Bluetooth stack thread and the stack goes and destroys the object (the last reference of it). When the application's thread gets scheduled again it would be stuck with a valid looking pointer which is in reality invalid.

[joerchan]

@zfviwx It is mentioned in the header here:
https://github.com/zephyrproject-rtos/zephyr/blob/master/include/bluetooth/conn.h#L283

Maybe it is unclear?

[zfviwx]

It would be much nicer if it mentions unref needs to be called🙂

[joerchan]

@zfviwx Is this clear: #23066?

[zfviwx]

Looks great! I'm impressed