Open jmealo opened 3 years ago
Historically AMQP support has been problematic in Node, I'm not sure if things have improved.
Historically AMQP support has been problematic in Node, I'm not sure if things have improved.
@mcollina: thank you for the quick reply :) I figured as much. I can provide an update on the current quality/status:
Microsoft is investing time into stress and smoke testing their upstream libraries that use rhea-promise
and working to improve the code quality of the azure-sdk for JS.
The Azure AMQP libraries have been overhauled, I hit issues during the customer preview that caused a production outage. We migrated to a function app and have had no issues since and zero incentive to risk moving things back now that the libraries are GA.
I would want to review recent commits to @azure/core-amqp
and rhea-promise
following Azure's smoke/stress test improvements before I used these libraries in a long-running process in production. I saw reports of, and experienced first hand, connection handling issues in long running processes.
AMQP defines a lot of behavior at the protocol level that the developer really needs to understand for a successful implementation. It's hard to get good ergonomics without hiding important details. I think that exposing more of the nature of the protocol via configuration options could be a good mix of ergonomics/exposure to the nature of the beast.
If you cut and paste the batch example code from the Azure docs for producers into an async request or event handler you're going to have orphaned batches/lost messages in production under load and a subtle memory leak. I opened an issue to improve documentation or add a helper object to eliminate the foot canon. The bug is unlikely to occur during development with the batch sizes a developer is likely to test with so I can't imagine my team is the first to hit this.
Here's a minimal fix that took me longer to come up with than I'd have liked:
const getBatch = async () => {
// If another batch is already pending; we return the promise that will resolve to that batch.
// You can .then() or await the same promise multiple times; you cannot resolve()/reject() multiple values
if (nextBatch) {
return nextBatch
}
try {
nextBatch = producer.createBatch({ maxSizeInBytes })
const batch = await nextBatch
return batch
} finally {
nextBatch = undefined
}
}
@mcollina: Do you think the foot canon with async batch creation in an event handler is worth doing a write up on to help folks understand?
Yes, it's definitely worth it.
I'm not sure what's the goal of this issue. Would you like to help in the mantainance of this module?
@mcollina: Hello!
I'm curious (as others may be):
Did you choose HTTP because you ran into issues with AMQP? I'm wondering what the pros/cons of using HTTP versus the azure-sdk with AMQP.
Thanks, Jeff