Bugzilla – Full Text Bug Listing
| Summary: | Remote services with Java primitives fail | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [RT] ECF | Reporter: | Bryan Hunt <bhunt> | ||||||
| Component: | ecf.remoteservices | Assignee: | Markus Kuppe <bugs.eclipse.org> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | major | ||||||||
| Priority: | P3 | CC: | bob.ziuchkovski, slewis | ||||||
| Version: | 3.4.0 | ||||||||
| Target Milestone: | --- | ||||||||
| Hardware: | PC | ||||||||
| OS: | Mac OS X - Carbon (unsup.) | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
(In reply to comment #0) > The root cause of the problem is in > org.eclipse.ecf.core.util.reflection.ClassUtil line 68: > > clazzA.isAssignableFrom(clazzB) > > will return false if one class is a primitive and the other is the object form > of the primitive. See also: I'd argue that ClassUtil behaves correctly. The problem rather appears to be that primitives get converted into the Object form during transport. Question is, if we make sure providers reverse the conversion prior to calling ClassUtil or if ClassUtil should take autoboxing into account and return a match anyway (assuming there is no exact better match)?! Created attachment 184109 [details]
A (examplary) test case
Created attachment 184110 [details]
mylyn/context/zip
Btw. org.apache.commons.lang.ClassUtils behaves exactly the same as the current implementation of ECF's org.eclipse.ecf.core.util.reflection.ClassUtil. (In reply to comment #1) > (In reply to comment #0) > > The root cause of the problem is in > > org.eclipse.ecf.core.util.reflection.ClassUtil line 68: > > > > clazzA.isAssignableFrom(clazzB) > > > > will return false if one class is a primitive and the other is the object form > > of the primitive. See also: > > I'd argue that ClassUtil behaves correctly. Well, I'd agree that ClassUtil behaves according to spec...but autoboxing was added to java after isAssignableFrom...so I'd also argue that isAssignableFrom isn't really spec'd properly given autoboxing. >The problem rather appears to be > that primitives get converted into the Object form during transport. Right...this is just autoboxing. It happens well before transport...it's done by the proxy. > Question is, if we make sure providers reverse the conversion prior to calling > ClassUtil or if ClassUtil should take autoboxing into account and return a > match anyway (assuming there is no exact better match)?! Couldn't we use Class.isPrimitive() (see http://download.oracle.com/javase/1.4.2/docs/api/java/lang/Class.html#isPrimitive%28%29) to have ClassUtil return match in the case of (e.g.) long isAssignableFrom Long? I'm not sure how we could require the providers to reverse the conversion. If isPrimitive is not usable or doesn't do the right thing then it probably requires sending meta-data about the class (whether it's a primitive or not). (In reply to comment #5) > Couldn't we use Class.isPrimitive() (see > http://download.oracle.com/javase/1.4.2/docs/api/java/lang/Class.html#isPrimitive%28%29) > to have ClassUtil return match in the case of (e.g.) long isAssignableFrom > Long? That's certainly possible but it bloats the implementation and needs extra consideration to return the closest/exact match first. E.g. class TestClass { public void foo(final Boolean b) {} public void foo(final boolean b) {} } > I'm not sure how we could require the providers to reverse the conversion. If > isPrimitive is not usable or doesn't do the right thing then it probably > requires sending meta-data about the class (whether it's a primitive or not). Btw. which provider shows this behavior? Does this happen for both r-OSGi as well as the generic provider? (In reply to comment #6) > Btw. which provider shows this behavior? Does this happen for both r-OSGi as > well as the generic provider? I'm using the generic provider. I have not tested with R-OSGi. (In reply to comment #7) > I have not tested with R-OSGi. Can you do so and test it with the generic provider? (In reply to comment #6) <stuff deleted> > > That's certainly possible but it bloats the implementation Do you mean just adds a little code to the method match matching? I'm not sure if I would characterize that as bloat. >and needs extra > consideration to return the closest/exact match first. E.g. > > class TestClass { > public void foo(final Boolean b) {} > public void foo(final boolean b) {} > } True...but again I'm not sure that this is so much of a problem...a couple of lines perhaps, but not a huge chunk of code. > > > I'm not sure how we could require the providers to reverse the conversion. If > > isPrimitive is not usable or doesn't do the right thing then it probably > > requires sending meta-data about the class (whether it's a primitive or not). > > Btw. which provider shows this behavior? Does this happen for both r-OSGi as > well as the generic provider? I think it will probably be shown by both (since r-osgi uses proxy autoboxing and object serialization as well for arguments), but it would be worthwhile testing/verifying this. (In reply to comment #9) > (In reply to comment #6) > <stuff deleted> > > > > That's certainly possible but it bloats the implementation > > Do you mean just adds a little code to the method match matching? I'm not sure > if I would characterize that as bloat. > > >and needs extra > > consideration to return the closest/exact match first. E.g. > > > > class TestClass { > > public void foo(final Boolean b) {} > > public void foo(final boolean b) {} > > } > > > True...but again I'm not sure that this is so much of a problem...a couple of > lines perhaps, but not a huge chunk of code. It's a few lines of code which have to be executed for each remote call. Given that, an extra bit on the network layer might a better option. Anyway, branch 331383 now contains a ClassUtil implementation that takes autoboxing into account. (In reply to comment #10) > > True...but again I'm not sure that this is so much of a problem...a couple of > > lines perhaps, but not a huge chunk of code. > > It's a few lines of code which have to be executed for each remote call. Sure...but since we're talking about a remote call the I/O performance is going to swamp any performance penalty associated with such code. Even the reflective Method lookup (getDeclaredMethods() is likely to swamp these calls I would guess. >Given > that, an extra bit on the network layer might a better option. Anyway, branch > 331383 now contains a ClassUtil implementation that takes autoboxing into > account. Ok...I'll take a look at it when I can (not right away). So isPrimitive() plus checks can/does work to deal with this issue? (In reply to comment #11) > Sure...but since we're talking about a remote call the I/O performance is going > to swamp any performance penalty associated with such code. Even the > reflective Method lookup (getDeclaredMethods() is likely to swamp these calls I > would guess. Without some hard numbers to back up both of our statements are guesswork. > Ok...I'll take a look at it when I can (not right away). So isPrimitive() plus > checks can/does work to deal with this issue? http://git.eclipse.org/c/ecf/org.eclipse.ecf.git/commit/?h=331383&id=4cf138ce33a599f69cd84e9bd4afc70051683f37 (In reply to comment #12) > (In reply to comment #11) > > > Sure...but since we're talking about a remote call the I/O performance is going > > to swamp any performance penalty associated with such code. Even the > > reflective Method lookup (getDeclaredMethods() is likely to swamp these calls I > > would guess. > > Without some hard numbers to back up both of our statements are guesswork. Agreed that we're guessing. But it's a pretty good guess that when it comes to remote calls any network I/O will on balance be an order of magnitude more costly (in time) than anything that happens locally. That's all I'm saying. > > > Ok...I'll take a look at it when I can (not right away). So isPrimitive() plus > > checks can/does work to deal with this issue? > > http://git.eclipse.org/c/ecf/org.eclipse.ecf.git/commit/?h=331383&id=4cf138ce33a599f69cd84e9bd4afc70051683f37 ok, ok :). (In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > > > Sure...but since we're talking about a remote call the I/O performance is going > > > to swamp any performance penalty associated with such code. Even the > > > reflective Method lookup (getDeclaredMethods() is likely to swamp these calls I > > > would guess. > > > > Without some hard numbers to back up both of our statements are guesswork. > > Agreed that we're guessing. But it's a pretty good guess that when it comes to > remote calls any network I/O will on balance be an order of magnitude more > costly (in time) than anything that happens locally. That's all I'm saying. True, but in this specific case we are not talking about a complete new remote call. It would just be an extra bit per parameter type that has a primitive counterpart. (In reply to comment #14) <stuff deleted> > > remote calls any network I/O will on balance be an order of magnitude more > > costly (in time) than anything that happens locally. That's all I'm saying. > > True, but in this specific case we are not talking about a complete new remote > call. It would just be an extra bit per parameter type that has a primitive > counterpart. Yeah :)...I get that...but all I'm saying is that if we add a constant...and likely pretty small...amount to the host-side invocation...it's likely to not be noticeable in most remote services use cases. And as this bug indicates, having support for autoboxing/primitive types in args for all providers that use ClassUtil will be a noticeable improvement for some remote services users. I stared at the code on the branch. It looks good to me. I haven't had a chance to setup/run the tests yet, but if it passes all tests (including the new one you created) then I would be in favor of merging to master for 3.4.1 and 3.5. Bryan, do you want this in ECF 3.5? FWIW: http://git.eclipse.org/c/ecf/org.eclipse.ecf.git/log/?h=331383 is the branch that has been created to test a possible fix. I have a workaround, so it's not critical, but it would be nice to get this fixed. (In reply to comment #18) > I have a workaround, so it's not critical, but it would be nice to get this > fixed. Have you tried the changes done in branch 331383? I've not tried the fix. I might be able to try it in 2 - 4 weeks. I believe this has been fixed. I just ran into this issue on the ECF 3.5.3 release. Per Scott's last comment, should this be fixed in 3.5.3, or is it fixed in a later commit? (In reply to comment #22) > I just ran into this issue on the ECF 3.5.3 release. Per Scott's last comment, > should this be fixed in 3.5.3, or is it fixed in a later commit? Hi Bob Could you describe what you are (not) seeing for your use case? (eg your interface sig...and what you are seeing at runtime? Also...what jre version you are using? Thanksinadvance As a matter of fact, the fix hasn't been merge from git branch 331383 [0] to master. I'm still waiting to receive feedback first. Markus [0] http://git.eclipse.org/c/ecf/org.eclipse.ecf.git/log/?h=331383 Thanks for the info. I was testing the simple IPOJO DOSGI tutorial (http://felix.apache.org/site/apache-felix-ipojo-dosgi.html) and adapting it to ECF. The exported service interface from this tutorial is as follows: public interface AdderService { int add(int a,int b); } The primitive handling for the ECF 3.5.3 release was causing the remote call to fail. Changing the interface to the following, however, works: public interface AdderService { Int add(Int a,Int b); } Per Markus' comment, I checked-out and built the 331383 branch. Using this branch, calls to the original primitive-based interface are successful. Merged into master with commit http://git.eclipse.org/c/ecf/org.eclipse.ecf.git/commit/?id=7375511c8a45957815e50551b075ceb04d08cab8 |
A remote service with functions that contain Java primitives as arguments cannot be called. For example: public interface MyService { void update(long value); } If you attempt to call update() remotely, you will get a NoSuchMethodException. The root cause of the problem is in org.eclipse.ecf.core.util.reflection.ClassUtil line 68: clazzA.isAssignableFrom(clazzB) will return false if one class is a primitive and the other is the object form of the primitive. See also: http://stackoverflow.com/questions/1650614/isassignablefrom-with-reference-and-primitive-types The workaround is to change all primitive arguments to their object form. For example: public interface MyService { void update(Long value); }