Pr/check destination null #1093
Conversation
|
Is there a any chance that any can review and merge my PR? And is there a plan to release a new patch version including this PR? For now we running our application with a debug version including the changes of this PR and we have not seen any exceptions so far. Thanks in advance. |
|
I'm interested to know why you had to defend against a null command in those places. Also bumping xUnit has created some new warnings. Could you fix those up please, so we have a clean build? Thanks. |
|
@drewnoakes, you are right, it was not where I got the actual exception. I fixed all the warnings and put all the task awaiting into a separate class and resolves all your findings. |
|
@drewnoakes did got any chance to take a look? With the last revert it is just removal of assert and oppgrade of System.ServiceModel.Primitives 4.10.3. Is there any chance to get en new patch release includeing this PR? Thanks in advance. |
tmatthey
force-pushed
the
pr/check-destination-null
branch
from
dc0a9db to
7ecaa5f
Compare
|
Any chance any could have a look again at this PR? I totally aware that this is an open-source on benevolent base, but I would really appreciate a new patch version. |
tmatthey
force-pushed
the
pr/check-destination-null
branch
from
7ecaa5f to
fb96e78
Compare
|
I did a reset and force push to regroup the my changes and make hopefully clear: one for the trey-catch, one for grade of NuGet of the lib code, and one for upgrading NuGet's and fixing unit tests. |
|
Thanks for your efforts on this @tmatthey - I am making a local NetMq build based on your branch while waiting for this to get reviewed. |
|
@follesoe Would be great if you could test too. I'm currently testing this branch in our environment with a local debug build, just to pickup any further asserts. |
|
I haven't seen any exceptions with a local debug build since my last commit 3 weeks ago. |
|
Me neither |
| // Get a command. | ||
| var ok = m_commandPipe.TryRead(out command); | ||
| Debug.Assert(ok); | ||
| return ok; | ||
| // We've got the signal. Now we can switch into active state if we can read. | ||
| m_active = m_commandPipe.TryRead(out command); | ||
| return m_active; |
There was a problem hiding this comment.
Can you help me understand why this is needed? The previous code was expecting this to always return true. Is there a race? Why is this a safe change?
| Assumes.NotNull(command.Destination); | ||
| command.Destination.ProcessCommand(command); | ||
| command.Destination?.ProcessCommand(command); | ||
| found = m_mailbox.TryRecv(0, out command); |
| // Try to prefetch a value. | ||
| if (!CheckRead()) | ||
| try | ||
| { | ||
| // Try to prefetch a value. | ||
| if (!CheckRead()) | ||
| { | ||
| value = default(T); | ||
| return false; | ||
| } | ||
|
|
||
| // There was at least one value prefetched. | ||
| // Return it to the caller. | ||
| value = m_queue.Pop(); | ||
| return true; | ||
| } | ||
| catch | ||
| { | ||
| value = default(T); | ||
| return false; | ||
| } | ||
|
|
||
| // There was at least one value prefetched. | ||
| // Return it to the caller. | ||
| value = m_queue.Pop(); | ||
| return true; |
There was a problem hiding this comment.
Adding try/catch can hurt performance. Catching all exceptions raises eyebrows. While this is defensive, I'd like to understand what the actual issue is in order to fix it. Is there a test we could write that reproduces this behaviour, for example?
|
I will revert to latest official NetMQ build and see if I can reproduce it. I ran into this issue numerous times on our Xamarin application, but switching to the fork from @tmatthey resolved many of the runtime issues. I know many of the problems I experienced was due to how the .NET process was suspended and resumed, where the .NET Sockets where rehydrated and in the incorrect state because the underlying Socket's in iOS/Android had been freed. If this was relevant for these particular changes I can't remember. Also, behavior might have changed on .NET 9 / MAUI which we are migrating to. |
Remove Debug.Assert(ok);, just return value of m_commandPipe.TryRead. I'm not certain if caller of Mailbox.TryRead may check if command.Destination to be not zero when processing the command, i.e., reading a command with CommandType.Done sent by ZObject.SendDone(), which has no destination.