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

Fix a few blocking/callback issues in the vault API #1486

Merged
merged 7 commits into from
Sep 19, 2023

Conversation

dgelessus
Copy link
Contributor

  • Rewrote VaultAgeAddDevice and VaultAgeSetDeviceInbox to use callbacks instead of blocking.
  • Removed some unused blocking vault functions.
  • Removed callback parameters that were never called.
  • Fixed names of functions that claimed to be blocking, but actually weren't.

This is only the low-hanging fruit. The remaining blocking vault calls are much harder to fix.

Unfortunately, the vault API still uses C-style callbacks with void* param, so the non-blocking implementations are much less straightforward. This would probably be much nicer with std::function and lambdas.

These callback parameters were ignored and no Python code defined a
publicAgeCreated or publicAgeRemoved method. The public/private change
notification is actually delivered as a plVaultNotifyMsg with type
kPublicAgeCreated or kPublicAgeRemoved sent by VaultSetAgePublic.
@Hoikas
Copy link
Member

Hoikas commented Sep 16, 2023

My concern here is that I think I already made everything nonblocking that could possibly be done without deep surgery in the Python code. For example, a very casual grep reveals this, which implies that the device will be available when the function call returns, and the device is immediately used in the next few lines. From what I can tell, this would error when this PR is merged. Further, I'm fairly certain that we can't just make the Python API's Age registration functionality nonblocking for the same reason. IIRC this is because PelletBahroCave is registered somewhere, then its vault is accessed immediately thereafter. I was hoping to address this with #1149, but limited time. I'm also not sure I like the Python API becoming less featureful, even though those features may not be used anywhere at this time.

@dgelessus
Copy link
Contributor Author

dgelessus commented Sep 17, 2023

For example, a very casual grep reveals this, which implies that the device will be available when the function call returns, and the device is immediately used in the next few lines. From what I can tell, this would error when this PR is merged.

Ah, you're right, I missed that - thanks. In this case, this can be fixed by moving the code from after the addDevice call into the callback, right?

The only other addDevice call is in xSimpleImager.py, and as far as I can tell, that one uses the callback correctly. I think I tested mainly with that one >.>

Further, I'm fairly certain that we can't just make the Python API's Age registration functionality nonblocking for the same reason. IIRC this is because PelletBahroCave is registered somewhere, then its vault is accessed immediately thereafter.

Yeah, I think that's one of the cases I noticed. That's why I didn't touch VaultRegisterOwnedAgeAndWait.

I'm also not sure I like the Python API becoming less featureful, even though those features may not be used anywhere at this time.

I can drop 9cce806 (the removal of ptVault.sendToDevice/VaultPublishNode) if you prefer. Edit: nevermind, that relied on VaultAgeSetDeviceInboxAndWait, so it would also need a full rewrite to callback style...

The other unused functions I removed already have non-blocking alternatives (and those are used somewhere), so no functionality is lost there.

@dgelessus
Copy link
Contributor Author

Fixed the code in nb01DRCImager.py, but I have no way to test it. That script seems completely unused - the hood classroom imager also uses xSimpleImager.py.

@Hoikas
Copy link
Member

Hoikas commented Sep 17, 2023

I can drop 9cce806 (the removal of ptVault.sendToDevice/VaultPublishNode) if you prefer.

I was more worried about the dropping of a few cbObject arguments that I saw. If there's another way to get those notifications in Python (eg OnVaultNotify()), then it should be fine.

@dgelessus
Copy link
Contributor Author

I explained that in the commit message for 9b8523c - those parameters were already dead:

Remove unused callbacks from PtCreatePublicAge/PtRemovePublicAge

These callback parameters were ignored and no Python code defined a publicAgeCreated or publicAgeRemoved method. The public/private change notification is actually delivered as a plVaultNotifyMsg with type kPublicAgeCreated or kPublicAgeRemoved sent by VaultSetAgePublic.

@Hoikas Hoikas merged commit 8009e89 into H-uru:master Sep 19, 2023
15 checks passed
@dgelessus dgelessus deleted the vault_less_blocking branch November 29, 2023 21:31
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