Copilot commented on code in PR #579:
URL: https://github.com/apache/pulsar-client-cpp/pull/579#discussion_r3287601672
##########
lib/ClientImpl.cc:
##########
@@ -244,36 +250,39 @@ void ClientImpl::handleCreateProducer(Result result,
const LookupDataResultPtr&
}
} catch (const std::runtime_error& e) {
LOG_ERROR("Failed to create producer: " << e.what());
- callback(ResultConnectError, {});
+ callback(error);
Review Comment:
In the exception path while constructing the producer, `callback(error)`
reuses the partition-metadata lookup `error` (which is `ResultOk` in this
branch). This will report success-but-with-error to the V2 callback, and the
legacy wrapper will translate it to `ResultOk` with an empty `Producer`.
Instead, propagate a failure `Error` that reflects the construction failure
(e.g., `ResultConnectError`/`ResultUnknownError` plus `e.what()`).
##########
include/pulsar/Client.h:
##########
@@ -108,7 +111,9 @@ class PULSAR_PUBLIC Client {
* @return ResultOk if the producer has been successfully created
* @return ResultError if there was an error
*/
- Result createProducer(const std::string& topic, const
ProducerConfiguration& conf, Producer& producer);
+ [[deprecated("use createProducerAsyncV2")]] Result createProducer(const
std::string& topic,
+ const
ProducerConfiguration& conf,
+
Producer& producer);
Review Comment:
The deprecation message on the synchronous `createProducer(...)` suggests
using `createProducerAsyncV2`, which changes the call semantics (sync → async)
and is not a drop-in replacement. Consider either providing a synchronous V2
API (e.g., returning `Error`/`Result+message`) or adjusting the deprecation
guidance to point to an equivalent synchronous alternative.
##########
lib/ClientImpl.cc:
##########
@@ -244,36 +250,39 @@ void ClientImpl::handleCreateProducer(Result result,
const LookupDataResultPtr&
}
} catch (const std::runtime_error& e) {
LOG_ERROR("Failed to create producer: " << e.what());
- callback(ResultConnectError, {});
+ callback(error);
return;
}
producer->getProducerCreatedFuture().addListener(
- std::bind(&ClientImpl::handleProducerCreated, shared_from_this(),
std::placeholders::_1,
- std::placeholders::_2, callback, producer));
+ [this, self{shared_from_this()}, callback{std::move(callback)},
producer](
+ const Error& error, const ProducerImplBaseWeakPtr&
producerBaseWeakPtr) {
+ handleProducerCreated(error, producerBaseWeakPtr, callback,
producer);
+ });
producer->start();
} else {
LOG_ERROR("Error Checking/Getting Partition Metadata while creating
producer on "
- << topicName->toString() << " -- " << result);
- callback(result, Producer());
+ << topicName->toString() << " -- " << error.result);
+ callback(error);
}
}
-void ClientImpl::handleProducerCreated(Result result, const
ProducerImplBaseWeakPtr& producerBaseWeakPtr,
- const CreateProducerCallback& callback,
+void ClientImpl::handleProducerCreated(const Error& error, const
ProducerImplBaseWeakPtr& producerBaseWeakPtr,
+ const CreateProducerV2Callback&
callback,
const ProducerImplBasePtr& producer) {
- if (result == ResultOk) {
+ if (error.result == ResultOk) {
auto address = producer.get();
auto existingProducer = producers_.putIfAbsent(address, producer);
if (existingProducer) {
auto producer = existingProducer.value().lock();
LOG_ERROR("Unexpected existing producer at the same address: "
<< address << ", producer: " << (producer ?
producer->getProducerName() : "(null)"));
- callback(ResultUnknownError, {});
+ callback(Error{ResultUnknownError,
+ "Unexpected existing producer for name " +
producer->getProducerName()});
Review Comment:
`producer` can be null after `existingProducer.value().lock()`. The log
message handles null, but the error message construction unconditionally
dereferences `producer->getProducerName()`, which can crash. Please guard
against null (or use a fallback name/address) when building the `Error` message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
