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

The problem with using Occurrences (for eventual Async Client Calls) #207

Open
drjdpowell opened this issue Dec 7, 2022 · 7 comments
Open

Comments

@drjdpowell
Copy link

drjdpowell commented Dec 7, 2022

The Client Sync Calls are well set up to eventually support an Async Client API, as they basically have "Start Action" parts, a "Wait till done" part, and a "Read Result part". However, the use of an Occurrence for the wait part will cause a bug whenever a new Request is made while an older one is not yet filled. This is because the Occurrence is static, and the same on each call, so the Response to the first Request will end the wait for both, causing the read of the second Response to be too early and thus fail.

2022-12-07 16_07_14-grpc-lvsupport-release lvlib_Client Unary Call vim Block Diagram on route_guide

Normally, I would use a Queue for these types of Waits, creating a new temporary Queue for each Request-Response, allowing multiple such async calls to be "inflight" at any one time. Would it be easy to switch to using a Queue?

@drjdpowell
Copy link
Author

BTW, Async Client-side code is far more interesting to me than the "Async" Server-side stuff that your scripted Server seems to have. I would actually recommend you drop that stuff as over-complicated and something that a User can do a number of ways if they need it. Client-side is far more important to think about.

@gregr-ni
Copy link
Collaborator

gregr-ni commented Dec 8, 2022

@drjdpowell
The "Async" server stuff is not async in the G sense. This is an overloading of the term and really just means supporting multiple requests to be handled at the same time. It does not change the async-ness of the G method implementation code. The current implementation really falls short of even parallel requests though. It allows multiple methods to be handled in parallel but requests for the same method still serialize.

Supporting parallel client requests only requires the VI you showed to be reentrant. Each reentrant instance will get its own occurrence so it is fine.

Supporting an async G client API is a different thing and I'm not sure it is warranted. My feeling is that in the vast majority of cases, a reentrant synchronous API is fine. For cases that really need an async G API then async call by ref can be used to wrap the API.

If you think it is important to support an async G client API, then I'd say make an issue supporting that enhancement with use cases and this is just an implementation detail of that support.

FYI @eyesonvis

@drjdpowell
Copy link
Author

It does not change the async-ness of the G method implementation code. The current implementation really falls short of even parallel requests though. It allows multiple methods to be handled in parallel but requests for the same method still serialize.

That's why I think you should drop it. It isn't bringing enough to the party to justify the increased difficulty in understanding. Most real-world applications are likely to have a one or two Procedures that take significant time, plus several very quick ones. Imagine, for example, an "Analyze Image" call; Users will want to analyze images in parallel, not to be able to other procedure types in parallel.

Also, doing stuff in parallel is a problem that has been solved in multiple ways already. You don't need to have that in scope.

@drjdpowell
Copy link
Author

For cases that really need an async G API then async call by ref can be used to wrap the API.

Some problems there:
One cannot ACBR call a Malleable VI, and Malleable VIs are your core technique interacting with your dll (and you can't use variants to solve this problem). So one is left with needing strong-typed cluster inputs on you Async-called subVIs. ACBR-capable VI references are vulnerable to Root-Loop Blocking, which requires all such References to be cached ahead of time (at least in anything that can claim to be quality software).

Anyway, my point is that is complicated, and causes a lot of design restrictions. I actually figured out last night how to work around the Occurrence issue, to get a Malleable API for an Async Client call, but I really had to call on some deep knowledge and experience to do so. It would be a lot easier if you used a Queue.

@drjdpowell
Copy link
Author

My feeling is that in the vast majority of cases, a reentrant synchronous API is fine.

Do you know any Actor Framework guys? Ask them what they think about blocking synchronous calls.

@AndrewHeim
Copy link
Contributor

I believe this is behind #272 and #320, and possibly #77.

Local loopback can go quickly enough that the reply comes and the occurrence gets fired before LabVIEW execution can resume and get to the occurrence. The result is a deadlock after a race condition.

I just noticed that there is a timeout that is unwired - I will submit that as a pull request since it (clunkily) prevents deadlock. It waits out the timeout and then can resume.

After seeing all this, I'm persuaded by @drjdpowell - we should strongly consider rewriting the client side. I like the idea of having a higher level call that makes everything easy. But

  1. We need something more graceful than this occurrence here
  2. Ideally we have a cleaner low level library that can be used for more advanced or asynchronous use cases. Auto-generating the protocol-related code (protobuf) is a fantastic productivity gain - but we shouldn't let that constrain more layers of our application than it has to. Where we wish to make additional assumptions to make an easy example for simpler users that's ok - but that needs to be another separate layer on top of the low level scripted libraries. The low level calls must not be coupled by those "simple use case" assumptions, or it will never see true adoption by customers NI cares about.

@drjdpowell
Copy link
Author

Note: swapping out the Occurrence for a temporary Queue or User Event should fix multiple problems without requiring more than minor changes.

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

No branches or pull requests

3 participants