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

Add flags option to nanomsg port msg calls. Make port_remove non-blocking. #195

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fruffy
Copy link
Contributor

@fruffy fruffy commented Jun 16, 2023

A little change to make sure clean up is somewhat non-blocking. There are some cases where cleanup can take a really long time. This is because __del__ tries to remove every single port synchronously, which gets slow for several hundred ports. Ideally, cleanup should be done in bulk by deleting all ports at once, but the current setup does not seem to support this.

Copy link
Member

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't that increase the probability of failure though?
If the message cannot be sent right away (maybe the peer is still consuming the previous message?), you will get an exception. It will interrupt clean up and the port won't be removed properly at the peer. Am I missing something?
Maybe you should use a thread pool instead to handle port removal in parallel.

@fruffy
Copy link
Contributor Author

fruffy commented Jun 16, 2023

True, not happy about this approach, but it seems important to make the delete call non-blocking.

Interesting, did not think about a thread pool. The problem is where would that thread pool be? The messages are sent every time __del__ is called, which can happen at any point (exceptions, exits, normal execution). So I guess it would need to be tied to whatever manages these objects, which is the DataPlane class?

@antoninbas
Copy link
Member

If you think making the call non-blocking will work well for your use case, then the less contentious and simplest option may be to add a command-line argument.
I believe that platform_args were meant for something like this originally:

ptf/ptf

Lines 285 to 287 in bb28a88

group.add_argument(
"-a", "--platform-args", help="Custom arguments for the platform"
)

You could consider reviving platform_args and adding one for the nn platform to toggle non-blocking port remove.

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.

2 participants