Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pr/check destination null #1093

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tmatthey
Copy link

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.

@tmatthey
Copy link
Author

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.

@drewnoakes
Copy link
Member

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.

@tmatthey
Copy link
Author

@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.

Process terminated. Assertion failed.
   at NetMQ.Core.Mailbox.TryRecv(Int32 timeout, Command& command)
   at NetMQ.Core.SocketBase.ProcessCommands(Int32 timeout, Boolean throttle, CancellationToken cancellationToken)
   at NetMQ.Core.SocketBase.GetSocketOptionX(ZmqSocketOption option)
   at NetMQ.NetMQSocket.GetSocketOptionX[T](ZmqSocketOption option)
   at NetMQ.NetMQSocket.get_HasIn()

@tmatthey
Copy link
Author

@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 tmatthey force-pushed the pr/check-destination-null branch from dc0a9db to 7ecaa5f Compare June 4, 2024 16:40
@tmatthey
Copy link
Author

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 tmatthey force-pushed the pr/check-destination-null branch from 7ecaa5f to fb96e78 Compare June 23, 2024 21:31
@tmatthey
Copy link
Author

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.

@follesoe
Copy link

follesoe commented Aug 4, 2024

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.

@tmatthey
Copy link
Author

tmatthey commented Aug 5, 2024

@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.

@tmatthey
Copy link
Author

I haven't seen any exceptions with a local debug build since my last commit 3 weeks ago.

@follesoe
Copy link

Me neither

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants