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

Please add timeout parameter for Messenger.GetAsync() method. #140

Closed
makercob opened this issue Jun 9, 2020 · 6 comments
Closed

Please add timeout parameter for Messenger.GetAsync() method. #140

makercob opened this issue Jun 9, 2020 · 6 comments

Comments

@makercob
Copy link

makercob commented Jun 9, 2020

For unreachable target devices, the Task of Messenger.GetAsync() cannot complete, which may cause memory leaks.

@theFra985
Copy link

theFra985 commented Oct 6, 2020

I think a CancellationToken parameter would be the best way to keep it compilant with the Task-based async pattern, but I think the problem depends on the System.Net.Sockets.Socket implementation that doesn't support CancellationTokens at the moment (see dotnet/runtime#33418)

@lextm lextm added the external label Oct 7, 2020
@lextm
Copy link
Collaborator

lextm commented Aug 22, 2021

.NET 6 does introduce many new APIs like this, so it is now possible to better control timeout and cancelation. However, such is limited to .NET 6 runtime and impossible to port to other legacy runtimes.

@DubyaDude
Copy link

DubyaDude commented Dec 1, 2021

This turned into quite a headache for me, and to be honest, this really needed a more obvious warning for anyone that would utilize this since it hasn't been properly fixed in more than a year.

I recoded part of our project as I inherited it from someone else, and decided to switch to async as it showed performance benefits. What happened to me was not only did this create a memory leak, but it kept UDP socket connections open every time it tried to contact a device that was offline (viewed through process explorer). This ended up domino-ing into all available ports being taken up, killing windows RDP, and killing DNS resolving on the server.

If anyone else had the issue, I would recommend compiling #164 and using that. It fixed the memory leak and closed the UDP socket when using the CancellationToken.

@lextm
Copy link
Collaborator

lextm commented Dec 1, 2021

@DubyaDude that's not really recommended. Like I commented there in the pull request, the only viable solution is to upgrade to .NET 6 which is work in progress, https://github.com/lextudio/sharpsnmplib/tree/net6

@DubyaDude
Copy link

DubyaDude commented Dec 1, 2021

@DubyaDude that's not really recommended. Like I commented there in the pull request, the only viable solution is to upgrade to .NET 6 which is work in progress, https://github.com/lextudio/sharpsnmplib/tree/net6

I understand it might not be a recommended fix and my apologies I didn't see the .NET 6 branch. However, it still would've been great to add some sort of warning in the meantime to the methods that have this issue for the people that are utilizing the NuGet package as this can cause some big issues for anyone who hasn't seen this.

@lextm lextm pinned this issue Dec 2, 2021
@lextm
Copy link
Collaborator

lextm commented Dec 2, 2021

@DubyaDude Developers using pre-.NET 6 based async Socket API (and all frameworks upon it) experience the same, so the issues are not limited to this specific library.

I have pinned this issue to the top of the list, but have no other plan.

@lextm lextm unpinned this issue Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants