search

H-uru / Plasma

Cyan Worlds's Plasma game engine
http://h-uru.github.io/Plasma/
GNU General Public License v3.0
203 stars 80 forks source link

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

Closed dgelessus closed 11 months ago

dgelessus commented 11 months ago

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.

Hoikas commented 11 months ago

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 commented 11 months ago

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 9cce806f84c780d90081c102999abc6bf0d80919 (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 commented 11 months ago

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 commented 11 months ago

I can drop https://github.com/H-uru/Plasma/commit/9cce806f84c780d90081c102999abc6bf0d80919 (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 commented 11 months ago

I explained that in the commit message for 9b8523cdf7493a2889e81f26abb884cb6a4d98c0 - 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.