Java plugin process #4239
Java plugin process #4239
Conversation
|
@markus2330 I had to create a separate plugin, because I also re-implemented the |
Can this new plugin be used to replace the old? Afaik the old one wasn't used anyway.
Library or plugin? Maybe we find a better name. |
The new plugin is more general, than the old one. It can work with any executable. For example, the For the old To replace the
Plugin. Suggestions for a better name are welcome. The current name comes from "Standard I/O" (or |
|
Please put the API changes to another branch, we do not have the time to review and discuss all of this. (@tucek also needs to be involved.) Please focus on allowing JNI plugins to run as robust as possible. We want to release this in February and there should be as little experimental code as possible. |
I can reduce the API changes, but it won't work with zero changes to the
It should be pretty robust already. I tried to keep the whole protocol as simple as possible. That's also why I used the
I assume you are talking about the I also want to emphasize that I didn't actually change any existing APIs. I only added new APIs. The old JNI interface is still there. |
kodebach
force-pushed
the
java-plugin-process
branch
from
793545c
to
e05586c
Compare
|
@markus2330 I have now rewritten this PR to use the existing Java Apart from tests/docs the PR is now fully read for review. |
src/bindings/jna/stdioproc/src/main/java/org/libelektra/stdioproc/StdIoProc.java
Outdated
Show resolved
Hide resolved
…elistPlugin to use these
|
Everything is now renamed. I didn't remove the |
|
This pull request introduces 1 alert when merging a1ea37e into a778788 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 0a613f7 into a778788 - view on LGTM.com new alerts:
|
|
There are still several problems with the build server. Can you please fix/rerun? Btw. did you check that everything gets installed? I'll test when merged with the packages. |
I'm not really sure, what's wrong. It works on my system, so I'll have to investigate a bit. From the error message it seems like Java can't find the
At least on my system everything gets installed, yes. I checked, because that's the only way to test |
|
This pull request introduces 1 alert when merging 3debbf1 into a778788 - view on LGTM.com new alerts:
|
|
I found the bug ( |
| Java Native Interface is a framework, which allows Java code to execute or to be executed by programs, written in other languages like C, C++ and Assembly. Most of Elektra’s plugins are written in C and C++. Developer can use the JNI plugin, which was created specifically for Elektra to write plugins using Java code. For more information on JNI plugins, please take a look [here](/src/plugins/jni/README.md). | ||
| To achieve this, the `process` plugin spawns a child process for the external executable and then uses a simple protocol to relay any requested operations to this child process. | ||
| The details of how this protocol works are not important for writing a Java plugin. | ||
| All the details of the protocol are abstracted via the `processApp` class and the `Plugin` interface. |
There was a problem hiding this comment.
I actually don't like the name ProcessApp. What about PluginProcess?
| All the details of the protocol are abstracted via the `processApp` class and the `Plugin` interface. | |
| All the details of the protocol are abstracted via the `ProcessApp` class and the `Plugin` interface. |
| Methods that are not supported, should simply return `Plugin.STATUS_SUCCESS`. | ||
| 2. In C the parent key of the contract depends on the plugins name. | ||
| For example, the contract for `dump` can be found under `system:/elektra/modules/dump` and the `dump` plugin returns it as such. | ||
| However, in Java the parent key for the contract is always `system:/elektra/modules/jni` (you may use the constant `Plugin.JNI_MODULE_CONTRACT_ROOT`). |
There was a problem hiding this comment.
This is confusing, as jni is not used. What about system:/elektra/modules/java?
There was a problem hiding this comment.
I just kept the old key name to minimize the number of changes.
| 1. In C it is possible to implement only `elektraPluginGet` and leave the other functions unimplemented. | ||
| Because of interface inheritance in Java, it is required to implement all 5 method `open`, `get`, `set`, `error` and `close`. | ||
| Whether a method is actually supported, must be communicated via the contract. | ||
| Methods that are not supported, should simply return `Plugin.STATUS_SUCCESS`. |
There was a problem hiding this comment.
Is throwing NoSuchMethodError fine, too?
There was a problem hiding this comment.
A method that is not marked in the exports/has part of the contract shouldn't be called. However, returning Plugin.STATUS_SUCCESS is still safer, because if the method is somehow called, simply nothing happens.
Also, if you want to throw something, the appropriate type would be UnsupportedOperationException. A NoSuchMethodError is a subtype of Error and therefore should not be caught and simply lead to a program crash.
There was a problem hiding this comment.
because if the method is somehow called, simply nothing happens.
Why would it be "somehow" called? If export/has = 0, we could guarantee that we do not call it? And then it would not matter how the implementation is. A typical implementation in Java would be to throw NoSuchMethodError, as calling such a method is basically an assertion error (which also extends Error).
The sentences as they are written now sound like we might call it, thus the user must implement it as Plugin.STATUS_SUCCESS. This is problematic from maintenance point of view, as someone might think the implementation is as it should be while in fact it is not implemented.
There was a problem hiding this comment.
A typical implementation in Java would be to throw NoSuchMethodError, as calling such a method is basically an assertion error (which also extends Error).
Every implementation I've seen would use UnsupportedOperationException in this case. AFAIK NoSuchMethodError is basically only used by the JVM itself when bytecode for classes is modified at runtime or when using reflection.
Thinking about it, the best solution might actually be to use default implementations in the interface:
public interface Plugin {
default int open(KeySet config, Key errorKey) {
throw new UnsupportedOperationException();
}
// no default because it must be implemented for the contract
int get(KeySet keySet, Key parentKey) throws KDBException;
default int set(KeySet keySet, Key parentKey) {
throw new UnsupportedOperationException();
}
default int error(KeySet keySet, Key parentKey) {
throw new UnsupportedOperationException();
}
default int close(Key parentKey) {
throw new UnsupportedOperationException();
}
}There was a problem hiding this comment.
My comment was only about these sentences requiring the user to return Plugin.STATUS_SUCCESS. Changing the interface sounds like a good idea but shouldn't be done for this release.
| @@ -0,0 +1,33 @@ | |||
| # Mounting Java Plugins | |||
|
|
|||
| Mounting a Java plugin is a bit more complicated than mounting a "normal" C plugin. | |||
There was a problem hiding this comment.
This is not a very welcoming sentence. Better you start with the simple wrapper and (if needed at all) explain how it works later.
| ``` | ||
|
|
||
| Here we added `/path/to/foo.jar` and `/path/to/bar.jar` to the classpath, so that the classes `org.example.Foo` and `org.example.Bar` can be found. | ||
| We can still use `org.libelektra.plugin.WhitelistPlugin`, because Elektra's JARs are always added to the classpath. |
There was a problem hiding this comment.
This is not jet a full tutorial: how to compile the plugin, ... The current information could be the man page of mount-java.
There was a problem hiding this comment.
Compiling the plugin isn't any different from compiling any other Java library into a JAR file. This is not supposed to be a Java programming tutorial, so I only detailed the parts that are special about Elektra plugins.
There was a problem hiding this comment.
As said: then simply put the information to the man page.
| * error, close) are implemented, even if they are not supported. Whether or not a method is | ||
| * supported, must be defined via the correspoding `exports/has` key of the contract. Any method | ||
| * that is not supported, should simply be implemented as `return Plugin.STATUS_SUCCESS`. | ||
| */ | ||
| public interface Plugin { | ||
|
|
||
| /** This is the root key of the JNI plugin wrapping a Java plugin for use by Elektra */ |
|
|
||
| try { | ||
| if (!protocol.handshake()) { | ||
| System.exit(1); |
There was a problem hiding this comment.
Since the handshake failed, the only real option here is to print something to stderr. However, that could be really confusing, because (under normal circumstances) we are not in the process that was called by the user.
Even if we print error messages to stderr (which could be really out of place depending on what the parent process is doing), in this specific case, I don't think we need one. If the handshake failed, there isn't much information we could provide. If the app was called by the process plugin and something went wrong, process will issue a "handshake failed" error. If the app was called in some other way, just exiting because the app is being misused seems fine.
There was a problem hiding this comment.
We could exit with 3 different error status? Then the plugin can wait for the child and knows the exit status for an error message.
There was a problem hiding this comment.
I'm not sure that would have much benefit. The three cases would be:
- "Handshake failed": Without any extra information the separate exit code is useless. The plugin already knows that the handshake failed, because it initiated the handshake which the child process aborted.
- An IOException: In this case, maybe a different error code could convey some information. The parent process probably also got some error from
read/writeing to its end of the pipes. - Another error during the protocol execution: This mostly boils down to an encoding/decoding error of the dump format. This shouldn't happen and if it does it's a bug.
Additionally, in all 3 cases the user can't really do anything to solve the error. Not even the developer of the Java plugin could do anything here. All these errors are essentially bugs that need to be reported and fixed by us (*).
Finally, any exit codes we us would need to be part of the protocol, so that other apps can use them too. This means defining which classes of errors use which code. All of this seems a lot of effort for very little benefit.
(*) I didn't use ELEKTRA_SET_INTERNAL_ERROR in the process plugin, because the errors are only internal errors, if the child app is also provided us (like with Java). If it's an external app, the developer of that app needs to fix the problem.
There was a problem hiding this comment.
"Handshake failed": Without any extra information the separate exit code is useless. The plugin already knows that the handshake failed, because it initiated the handshake which the child process aborted.
It does not know if the child agrees about that the handshake failed. Even if this is very unlikely/impossible there is also not a good reason to use the same exit code?
An IOException: In this case, maybe a different error code could convey some information. The parent process probably also got some error from read/writeing to its end of the pipes.
Good idea!
Additionally, in all 3 cases the user can't really do anything to solve the error. Not even the developer of the Java plugin could do anything here. All these errors are essentially bugs that need to be reported and fixed by us (*).
Yes, so the error should be designed that it is helpful for us.
There was a problem hiding this comment.
It does not know if the child agrees about that the handshake failed.
What do you mean "the child agrees"? It is a handshake. By definition, it only succeeds if both parties agreed on the success. If the parent starts with ELEKTRA_PROCESS INIT v1 and the child does not respond exactly as expected (ELEKTRA_PROCESS ACK v1 etc.), then the handshake has failed by definition.
There is also not a good reason to use the same exit code?
Yes, there is. Like I already said, it would require a strict definition of which error classes should use which error codes, because this is a protocol that anybody could implement. This not necessarily hard, but its a lot of effort (probably involving some further discussion) that I'd really like to avoid, since I don't really see the benefit.
On a more technical note, it also requires quite extensive changes to the process plugin's code. Currently the parent just tries to read/write from/to the pipe (via freadks, fwriteks, getline, fprintf, etc.). If this fails, it reports an error. If the child exited (for what ever reason) the pipes will be closed and those read/write calls fail. I'm not even sure how you'd check the exit code after a fail read/write. It probably involves some complicated signal capturing logic.
Yes, so the error should be designed that it is helpful for us.
See the footnote. Also every error that is reported already has a unique reason, so we know already in which exact line of process.c the failure occurred.
There was a problem hiding this comment.
What do you mean "the child agrees"?
The child might have written the ACK to stdout but the parent process does not receive it (e.g. wrongly connected stdout).
On a more technical note, it also requires quite extensive changes to the process plugin's code.
If it makes the code much more complicated then leave it. We will see if errors in the protocol occurs and if we will be able to debug it easily 😉
| } | ||
|
|
||
| if (!protocol.run()) { | ||
| System.exit(1); |
| System.exit(1); | ||
| } | ||
| } catch (IOException e) { | ||
| System.exit(1); |
There was a problem hiding this comment.
Some error message? Send error via protocol?
There was a problem hiding this comment.
Where would this error message go? The IOException means that something went wrong while communicating with the parent process. Sending an error message via the protocol is clearly not an option here.
For more about errors see above.
| char buf_; | ||
| }; | ||
|
|
||
| int freadks (KeySet * ks, FILE * file, Key * errorKey) |
There was a problem hiding this comment.
rename to fserialize, funserialize (also rename serialise and unserialise)
There was a problem hiding this comment.
also rename serialise and unserialise
Why?
There was a problem hiding this comment.
Because it is still British English and we decided to go for American English.
There was a problem hiding this comment.
Oh okay, I thought you didn't like the term serialize/unserialise for some reason
Changes
Adds a new plugin
stdioprocthat can be used to put plugins into a separate process (an alternative toprocess). Also adds first implementation in the JNA binding.Closes #3891
Basics
(added as entry in
doc/news/_preparation_next_release.mdwhichcontains
_(my name)_)Please always add something to the release notes.
(first line should have
module: short statementsyntax)close #X, are in the commit messages.doc/news/_preparation_next_release.mdscripts/dev/reformat-allChecklist
(not in the PR description)
Review