-
Describe the bugWhat version of protobuf and what language are you using? Windows 11 Method: Any.Parser.ParseFrom().Unpack Errors Observed
During testing I noticed only the Client to Server (Protobuf) was erroring and not the Server to Client (Avro) so I switched to using Avro for both and it resolved the issue. Since I'm not sure where this issue lies I'm submitting the to both the RabbitMQ and Protobuf repositories Reproduction steps
Expected behaviorNo Errors Additional contextOne additional note, of the 10 different brokers I tested only RabbitMQ suffered from this issue. The source code for this test is available here |
Beta Was this translation helpful? Give feedback.
Replies: 9 comments 17 replies
-
|
While I appreciate you providing code, I have no idea how to run it to reproduce this behavior, nor do I have time to review such a large amount of code. If you can demonstrate this issue using a single console application (which should be possible), please do so. More than likely you are not using this library or RabbitMQ correctly, or something is not working like you expect. |
Beta Was this translation helpful? Give feedback.
-
|
Here's a sample log measuring req/sec, 32 connections using .NET crank, 1 hour duration. If it's not used correctly why are there 6M requests that are successful? Stats |
Beta Was this translation helpful? Give feedback.
-
Again, you're asking me to spend a significant time debugging something that is probably (even by your own words) not an issue with this library or RabbitMQ. As I said before, if there is an easy way to reproduce this, even if I have to run something for a long period of time, please provide code and instructions to do so. |
Beta Was this translation helpful? Give feedback.
-
is not a part of this codebase. For RabbitMQ and this client, message bodies are opaque sequences of bytes.
There could have been be 666M messages, it does not prove anything beyond the fact that it is a behavior that's difficult to reproduce, timing- and data sequence-sensitive issue. That has nothing to do with this client or RabbitMQ in all likelihood. |
Beta Was this translation helpful? Give feedback.
-
|
@gradx this client has a ≈ 17 year old history and amongst hundreds of issues discovered, reported and addressed I do not recall any cases of incorrect serialization of AMQP 0-9-1 frames since ≈ 2013. You must narrow this down to a specific encoded payload and prove that it is this client (or RabbitMQ server) that is somehow responsible for this behavior, which is very likely not the case because message payloads are just byte sequences for this client and RabbitMQ. It is not up to us to prove that no issue exists. It is up to you to prove its presence. Only paying users or regular contributors can count on the kind of engagement you seem to be seeking here. I don't recall you being a contributor, and if you have a support subscription, please open a ticket via the support portal. Even then, we'd ask you to narrow this down to a specific payload
"But I can publish millions of messages successfully" proves absolutely nothing except for the fact that this issue is timing- or input sequence-specific. |
Beta Was this translation helpful? Give feedback.
-
|
Reproducing the payload is easy since it's Protobuf. Definitionhttps://github.com/gradx/GenieDotNet/blob/main/GenieDotNet/SharedFiles/Protos/shared.proto Mock objecthttps://github.com/gradx/GenieDotNet/blob/main/GenieDotNet/Genie.Common/Web/MockPartyCreator.cs I'm not here seeking support but rather to report an issue I found while testing various brokers and to bring attention to the matter. Since you said yourself you haven't seen this issue since 2013 and the community here is a bit defensive I'll let a paying customer find the issue and report it. |
Beta Was this translation helpful? Give feedback.
-
|
This line suggests that publishing is performed with an object pool of some kind. This very likely means that a shared channel ( I won't spend any more time guessing because guessing is a very expensive way of troubleshooting distributed systems and binary protocol/codec implementations. |
Beta Was this translation helpful? Give feedback.
-
I'll remove the object pooling and see if it resolves the issue but from testing I believe it was there before it was added. |
Beta Was this translation helpful? Give feedback.
-
Microsoft.Extensions.ObjectPool is part of the ASP.NET Core infrastructure that supports keeping a group of objects in memory for reuse rather than allowing the objects to be garbage collected. All the static and instance methods in Microsoft.Extensions.ObjectPool are thread-safe. Just an FYI, I removed it to appease you but I was already aware it is threadsafe |
Beta Was this translation helpful? Give feedback.
-
|
If someone wrote a large C application, that, after running in production for a while, crashed with a segmentation fault, do you think it's reasonable to suspect
Then pick one of these other brokers, and create a project that will demonstrate this issue with RabbitMQ, and that it doesn't happen with one of these other brokers. |
Beta Was this translation helpful? Give feedback.
-
The project already includes every broker and it's easy to toggle between each. I haven't created the instructions yet because it involves Docker instances but that's next on my list. So I made the following changes to the Command Handler And performance now suffers tremendously (which was why I added object pooling to begin with) and the issue still remains. Error:Google.Protobuf.InvalidProtocolBufferException: While parsing a protocol message, the input ended unexpectedly in the middle of a field. This could mean either that the input has been truncated or that an embedded message misreported its own length. I'll create a project that isolates this issue to RabbitMQ only by tomorrow. One other important note. The Consumer is also multithreaded: Using TPL Dataflow and a message pump: Should I remove this too? |
Beta Was this translation helpful? Give feedback.
-
|
If you'd like to assist this project, and possibly resolve this issue, you could update your code to use the latest version 7 RC - https://www.nuget.org/packages/RabbitMQ.Client/7.0.0-rc.8
If you use version 7.0.0-rc.8, and do not experience this issue, I will buy you a coffee. |
Beta Was this translation helpful? Give feedback.
-
I upgraded to 7.0.0-rc8 and tested the command handler with and without pooling and the error still persists. I'll have to create an entirely new RabbitMQService for an e2e single-threaded test |
Beta Was this translation helpful? Give feedback.
-
The single threaded test ran without errors for 15 minutes (20k requests). What you're proposing basically eliminates the possibility of hosting RabbitMQ in a web service? I'll let it run overnight for 8 hours to see what happens. Details
|
Beta Was this translation helpful? Give feedback.
-
|
I've attached a simplified project that removed all the unnecessary code. Once you've installed .NET crank you can run start_crank.bat after starting the IngressConsumer. |
Beta Was this translation helpful? Give feedback.
-
|
Thank you, I have added it here - https://github.com/lukebakken/rabbitmq-dotnet-client-1660 I will try to find time to look at this in detail by next Friday. |
Beta Was this translation helpful? Give feedback.
-
|
@gradx I have invited you as a collaborator in that repository. |
Beta Was this translation helpful? Give feedback.
-
I don't know what you mean here. |
Beta Was this translation helpful? Give feedback.
-
There's evidence of the contrary that spans 17 years. How you specifically use this client in a Web app may or may not be optimal or fit something else you are trying to do but there's ample evidence that this client is used in .NET-based "Web services." This client's docs have stated for over decade that there are known and intentional concurrency limitations. The entire .NET community was able to deal with this limitation that primarily affects publishing. |
Beta Was this translation helpful? Give feedback.
-
|
After working with @lukebakken he helped me discover that BasicDeliverEventArgs is not thread-safe. Perhaps BasicDeliverEventArgs .Body should be marked as Obsolete and a descriptor added to indicate how to handle it better similar to how the RecyclableMemoryStream does so any references display an obvious issue: /// <summary>
/// Returns a new array with a copy of the buffer's contents. You should almost certainly be using <see cref="GetBuffer"/> combined with the <see cref="Length"/> to
/// access the bytes in this stream. Calling <c>ToArray</c> will destroy the benefits of pooled buffers, but it is included
/// for the sake of completeness.
/// </summary>
/// <exception cref="ObjectDisposedException">Object has been disposed.</exception>
/// <exception cref="NotSupportedException">The current <see cref="RecyclableMemoryStreamManager"/>object disallows <c>ToArray</c> calls.</exception>
/// <exception cref="OutOfMemoryException">The length of the stream is too long for a contiguous array.</exception>
#pragma warning disable CS0809
[Obsolete("This method has degraded performance vs. GetBuffer and should be avoided.")]
public override byte[] ToArray()
{
this.CheckDisposed();
string? stack = this.memoryManager.options.GenerateCallStacks ? Environment.StackTrace : null;
this.memoryManager.ReportStreamToArray(this.id, this.tag, stack, this.length);
if (this.memoryManager.options.ThrowExceptionOnToArray)
{
throw new NotSupportedException("The underlying RecyclableMemoryStreamManager is configured to not allow calls to ToArray.");
}
var newBuffer = new byte[this.Length];
Debug.Assert(this.length <= int.MaxValue);
this.InternalRead(newBuffer, 0, (int)this.length, 0);
return newBuffer;
} |
Beta Was this translation helpful? Give feedback.
-
If I were to submit a PR, I would add a MessagePump since during testing I noticed TPL Dataflow was 2-3x faster than using a thread pool or task.
@lukebakken mentioned he had some ideas on how to improve the performance so once I hear back from him I'll consider the next steps. |
Beta Was this translation helpful? Give feedback.
-
|
@gradx I was only talking about a small doc PR. |
Beta Was this translation helpful? Give feedback.
-
|
It's not that This affects all message bodies that are returned by RabbitMQ, not just those exposed via the eventing consumer. We'll make sure that the documentation in the code blocks and other places are more clear about this.
Feel free to add an example. In your particular code, you're using a wait event when I think something like a |
Beta Was this translation helpful? Give feedback.
-
Would it be possible to implement IDisposable and throw a more specific exception when the memory been reclaimed? |
Beta Was this translation helpful? Give feedback.
-
Not without wrapping the underlying memory in some other class, which would increase allocations and decrease performance. That's the sort of thing a higher-level library might do. I'll make sure the in-line XML docs (and other docs) are clear about this. |
Beta Was this translation helpful? Give feedback.
After working with @lukebakken he helped me discover that BasicDeliverEventArgs is not thread-safe. Perhaps BasicDeliverEventArgs .Body should be marked as Obsolete and a descriptor added to indicate how to handle it better similar to how the RecyclableMemoryStream does so any references display an obvious issue: