Events support: Java API#124
Events support: Java API#124dhouck wants to merge 6 commits intoNorth-Western-Development:1.20.1from
Conversation
dhouck
force-pushed
the
events-java-api
branch
2 times, most recently
from
561c2c8 to
aa806d4
Compare
|
I have what I believe to be a good solution to the potential for missing events, with a fully backwards-compatible HLAPI change (adding an extra field besides The other TODOs are at least partly blocked on my understanding of I hope to look into both within the next few days. |
|
Oh, as per the comment added in the third commit, there can be problems in crmode, if subscriptions are still active when the bus FD is closed. I donʼt think any userspace API uses that, and again it wonʼt cause any issues if nothing is subscribed to, but worth an explicit mention here. |
|
(Fixed some whitespace in the latest commit) |
|
Added 571f7f7, because it makes more sense for the bus (which already has the gson object and already does conversion for everything else including method results) to convert event data to JSON. As I understand it (and this is certainly the case in my tests), the gson library will just do the right thing if the passed object is already a |
|
Update: the issue is fixed but see below for discussion of the fix. Do not merge; I found a bug that happens specifically when upgrading from before this to after (because I think I have a fix, and itʼll be tested in a few minutes. |
This makes events almost fully supported. However, there is still a bug with unsubscribing and event-only devices without methods are not yet supported. This change requires switching to a single buffer, like with the transmitBuffer, although this one occasionally needs to be reallocated to support large messages. The max message size was previously only enforced for messages from the computer to the bus, but messages from the bus to the computer could be arbitrarily large if, for example, there were a lot of devices hooked up and you made a `list` call. Now messages of arbitrary size are still supported, but sending lots of small messages without clearing the buffer (as might happen if the computer subscribes to something then never reads the events) won't grow memory use indefinitely. Because ByteBuffer isn't thread-safe this also synchronizes the reading and writing to avoid issues; since you can't safely re-assign the locking object this also requires implementing a new object as the lock for the receive buffer. It is possible that switching to a different data structure would avoid this, but none seemed suitable given the need to occasionally grow for large messages but not to grow automatically for lots of small messages.
This allows us to fix a bug with unsubscribing from ObjectDevices, and to have an RPCDevice with no methods which only generates events. For most RPC devices, either they are an event source, or they are not, and `device instanceof RPCEventSource source` either works or gives null, as appropriate. But in some cases, such as for. ObjectDevice, the RPCEventSource and the RPCDevice are different objects, so this can be overridden. Related to this, unsubscribing was broken for ObjectDevices, because RPCDeviceBusAdapter::unsubscribe assumed the simpler line was sufficient. Now both subscribe and unsubscribe use the asEventSource method, and are otherwise simpler and less likely to get out of sync. Additionally, this makes the check of whether a DeviceList supports subscription a single line, which makes it easy to check when populating all the devices on the bus, and lets us easily support event-only devices with no methods. This should be rare (even a device primarily intended to present events can have methods for checking state), but there are a few use cases. To the extent subscriptions were previously supported, this is a backwards-compatible change; if something worked as an event source before (with or without having issues with unsubscribing), it still should.
We already use the bus's gson instance for method results; we should for event data also.
When a virtio console device that acts as a TTY, like the current RPC bus implementation, is opened, the Linux kernel resets termios and re-enables echo. This is usually not a problem, unless a device is left subscribed when the bus FD is closed and tries to send a message, which will then be echoed back. This is a hacky workaround that mostly detects and avoids the problem, but hopefully will be replaced by at least one of the two better suggestions. As part of that hope, this also removes mention of /dev/hvc0 except as necessary for existing scripts, to make it easier to move in the future.
Previously if a message was dropped, there was no way to know. This gives every message from the bus to the VM a sequence number; if any is ever skipped, that tells the VM that some messages were not received. This should rarely happen unless there is a flood of events, or a few very large events, but if it does happen it's good to be able to know. So far, no userspace library actually handles these sequence numbers, but as far as I can tell none of the existing libraries are broken by them either.
1 participant
Description
This is the Java side of my ongoing project to finish the partial support for events and subscriptions thatʼs in the code.
The main problem was that the existing code does not allow more than one message in the queue for the VM to receive, and would raise an exception if there was more than one, which could easily crash the server. My initial thought was to still have the limit but discard extra messages, but often more than one event at a time is useful (especially with the redstone interface block, given the way that redstone dust has excessive amounts of block updates when it powers down), so I did a more complicated thing instead.
There are a few other problems with the Java side of events that I also fixed:
unsubscribedidnʼt work withObjectDevices, the way the RPC bus is hooked up to the VM causes extra garbage the bus didnʼt know how to handle (which I have a hacky fix for and a couple ideas about better fixes which can be implemented separately later), the bus would ignore devices which only have events and not methods, and finally there was a chance messages would be silently dropped if there were too many events.Other work
I am also working on making the Python library better (I can maybe work on the Lua library after that but I do not know Lua nearly as well), but that will be a different PR. The goal is that as long as subscriptions are not being used, everything here stays exactly the same, and the existing library can still work.
My existing Python PR, #122, is for things that make sense even without events, even if most of it is unlikely to come up; the future PR will have things like
Device.subscribeandDevice.unsubscribemethods.I also have plans to improve the redstone interface events in a few ways, but that is also a separate PR; for now it generates many redundant events and the side identification is global for those events even though itʼs local for all the method calls.
The medium-term solution to the echo problem requires adding a program to the Minux
rootfs-overlayand setting it up to launch at system startup; I have most of the program written but have not finished it.If the long-term solution to the echo problem is adopted, it will require a change to the Sedna
VirtIOConsoleDeviceclass; I have a proof-of-concept change there but it needs work. It will also require any external projects which assume the location of the bus devices to be updated; with the current hacky solution or the medium-term less-hacky one, projects will only need to be updated if they want to support events.Specific things to call out
This changes the on-the-“wire” format in two ways, both of which should be mostly backwards compatible but which should be made explicit:
seqfield, in addition totypeanddata. This is alongwhich increments for each message, making it easy to see when messages are missed. Currently no library checks it, of course, but this could be very useful if it comes up.subscribeandunsubscribemethods from the VM now get a response like every other VM-to-bus message does; previously they only got a response on error. The success message is an empty message of the same type, which somewhat mirrors the waylistandmethodsmessages work nowAlso, I am not used to Java and may have made bad assumptions, especially around concurrency. I donʼt think I introduced any concurrency issues, but that should be double-checked. Iʼm particularly unsure about
RPCDeviceBusAdapter::reset; it looks like that might have had issues already, although I havenʼt caught it acting up, so Iʼm not sure if that is handled properly or if I might have made it worse.