[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
On 2012/06/11 22:56:06, scottb wrote: Rajeev and I went through and made some rough comments Damn invalid XSRF token. You don't get as nice of a comment this time. Since I can't write to this issue, I made a new issue here, with patchset 1 being Yi's work that you reviewed, rebased against trunk, and then patchset 2 being my changes to address your feedback: http://gwt-code-reviews.appspot.com/1760803 Removing false_nowarn was nice, made a few things cleaner--the only thing that would be cleaner still would be just removing the options all together and always serializing final fields. :-) http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
zhuyi, do you have time to look at Scott and Rajeev's feedback? If not, let me know and I can take a crack at it sometime in the next few days. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
Hi Stephen, I am very busy with my current project. Sorry I do not think I have time to work on it right now. Thanks! Yi On Tue, Jun 12, 2012 at 8:24 AM, stephen.haber...@gmail.com wrote: zhuyi, do you have time to look at Scott and Rajeev's feedback? If not, let me know and I can take a crack at it sometime in the next few days. http://gwt-code-reviews.**appspot.com/1380807/http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
Was debating whether this one should go in for GWT 2.5, but my inclination is to skip it. Please let me know if anyone feels strongly about this. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
On Fri, May 4, 2012 at 12:15 PM, rda...@google.com wrote: Was debating whether this one should go in for GWT 2.5, but my inclination is to skip it. Please let me know if anyone feels strongly about this. http://gwt-code-reviews.**appspot.com/1380807/http://gwt-code-reviews.appspot.com/1380807/ It has been waiting a few years, another one won't hurt. -- John A. Tamplin Software Engineer (GWT), Google -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
If it's important enough, we should land it, but my impression is that it isn't It is the 7th-ranked bug in the issue tracker (by number of stars). But, as John said, it's had 5 years to get all those votes. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
it looks like there's disagreement on the implementation My recollection is that zhuyi's original patch used a static variable to store the do we serialize final fields or not state. However, the variable is a per-module configuration, so putting it as static could have lead to non-determinism if multiple modules were getting compiled at the same time. I took zhuyi's work, killed the static, and instead changed various methods to pass around the context so that serialize final fields or not could be turned on/off at the right place. zhuyi incorporated that into this patch, so, no statics, so I think we're good to go. (Correct me if I'm wrong, John.) My only thought was that I wanted to go further and *always* serialize final fields, which would be cleaner as we could avoid the somewhat complex conditional behavior, but that was deemed to much of a breaking change. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
On Fri, May 4, 2012 at 2:44 PM, stephen.haber...@gmail.com wrote: it looks like there's disagreement on the implementation My recollection is that zhuyi's original patch used a static variable to store the do we serialize final fields or not state. However, the variable is a per-module configuration, so putting it as static could have lead to non-determinism if multiple modules were getting compiled at the same time. I took zhuyi's work, killed the static, and instead changed various methods to pass around the context so that serialize final fields or not could be turned on/off at the right place. zhuyi incorporated that into this patch, so, no statics, so I think we're good to go. (Correct me if I'm wrong, John.) My only thought was that I wanted to go further and *always* serialize final fields, which would be cleaner as we could avoid the somewhat complex conditional behavior, but that was deemed to much of a breaking change. I haven't looked at Zhu's work in a year and a half, but my recollection was the static issue was the only problem (and perhaps needing more tests, but I don't remember when that was relative to later development). -- John A. Tamplin Software Engineer (GWT), Google -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
(and perhaps needing more tests, but I don't remember when that was relative to later development). Yeah, seems like that may have initially been the case, but by the time I got involved, I think he had some good tests in place. I see a number of FinalFieldsXxxTest in the review that I remember being helpful while working on it. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
I meant to comment; this would all be a lot simpler if GWT-RPC could serialize final fields as the default/only behavior (basically remove any configuration variables to turn it on/off). I understand this isn't viable for the next release, as it's a breaking change, and users might be putting values in final fields that they don't want to go over the wire (although hopefully that is rare). But, just throwing it out there, would it be possible to deprecate all of the configuration variables, and say that in the next major release, final fields will always be serialized? Then a lot of the conditional flags and logic could be removed, and the implementation would be simplified. Which isn't critical, but I think would be nice. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
Not really practical. On Tue, Oct 4, 2011 at 10:35 AM, stephen.haber...@gmail.com wrote: I meant to comment; this would all be a lot simpler if GWT-RPC could serialize final fields as the default/only behavior (basically remove any configuration variables to turn it on/off). I understand this isn't viable for the next release, as it's a breaking change, and users might be putting values in final fields that they don't want to go over the wire (although hopefully that is rare). But, just throwing it out there, would it be possible to deprecate all of the configuration variables, and say that in the next major release, final fields will always be serialized? Then a lot of the conditional flags and logic could be removed, and the implementation would be simplified. Which isn't critical, but I think would be nice. http://gwt-code-reviews.**appspot.com/1380807/http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
On 2011/09/14 17:45:17, zhuyi wrote: Ray, John and others: I and Stephen have been working on this for a while to make the option of serializing final fields in RPC be a non-static variable to eliminate flakiness, and pass this option to server side by serialization policy file. Please let us know if the change is in good shape and ready for submission. Thanks. Yi Please let us know if you have any comments. Thanks. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
Ray, John and others: I and Stephen have been working on this for a while to make the option of serializing final fields in RPC be a non-static variable to eliminate flakiness, and pass this option to server side by serialization policy file. Please let us know if the change is in good shape and ready for submission. Thanks. Yi http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
On 2011/09/07 18:03:04, zhuyi wrote: On 2011/09/07 18:00:10, zhuyi wrote: I test this through the entire test suite. It still broke one case (testEnumArray): http://code.google.com/p/google-web-toolkit/source/browse/trunk/user/test/com/google/gwt/user/client/rpc/CollectionsTest.java?r=8394#268 It seems that something is not correct with the Enum type. I was unable to reproduce this failure--how are you invoking ant? Perhaps I'm doing something wrong. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
On 2011/09/07 18:00:10, zhuyi wrote: I test this through the entire test suite. It still broke one case (testEnumArray): http://code.google.com/p/google-web-toolkit/source/browse/trunk/user/test/com/google/gwt/user/client/rpc/CollectionsTest.java?r=8394#268 It seems that something is not correct with the Enum type. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
http://gwt-code-reviews.appspot.com/1380807/diff/50002/user/test/com/google/gwt/user/RPCSuite.java File user/test/com/google/gwt/user/RPCSuite.java (right): http://gwt-code-reviews.appspot.com/1380807/diff/50002/user/test/com/google/gwt/user/RPCSuite.java#newcode110 user/test/com/google/gwt/user/RPCSuite.java:110: suite.addTestSuite(FinalFieldsFalseNoWarnTest.class); I don't think FinalFieldsFalseTest and FinalFieldsFalseNoWarnTest made it into patchset 7. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
Here is my take on putting the final field switch into the policy file: https://github.com/stephenh/scalagwt-gwt/commit/35535b2f36cb196792a411430567a7a9aeb8adb1 Now final fields only go across the wire if the user opts in, and the server-side behaves accordingly. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
On 2011/09/03 01:04:50, zhuyi wrote: Integrated Stephen's change on SerializationPolicy. Individual test cases works fine, but test cases were broken when they were put together (e.g. run RPCSuite.java). That is caused by the caches in SerializabilityUtil.java. I've temporary disabled two caches in that file, and it worked fine (please see applyFieldSerializationPolicy and getSerializationSignature methods in SerializabilityUtil.java). We need to add SerializationPolicy as another key to lookup the caches. Any good suggestion on the cache structure improvement? http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
caused by the caches in SerializabilityUtil.java Huh, good catch. Any good suggestion on the cache structure improvement? Using a Pair seems to work: https://github.com/stephenh/scalagwt-gwt/commit/9dd82d23d6bdf904e92af5502a3eba450c615c63 http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
Thinking about it some more; it would be the easiest if GWT just started serializing final fields across the board, and only skipped them if they were marked with a GwtTransient. That is basically what the current patch does (except for ignoring sets on the client-side). Anything else requires making the server-side aware of the rpc.final.serialize value. Maybe that is easier than I'm imagining. Anyway, it'd be a breaking change, so perhaps it's not an option, but I just thought I'd mention it. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
On Thu, Sep 1, 2011 at 11:56 AM, stephen.haber...@gmail.com wrote: Thinking about it some more; it would be the easiest if GWT just started serializing final fields across the board, and only skipped them if they were marked with a GwtTransient. That is basically what the current patch does (except for ignoring sets on the client-side). Anything else requires making the server-side aware of the rpc.final.serialize value. Maybe that is easier than I'm imagining. Anyway, it'd be a breaking change, so perhaps it's not an option, but I just thought I'd mention it. There is a risk that existing users were relying on current behavior to not send sensitive data in final fields across the wire, and changing it without some form of opt-in seems a security risk. It could be adding an inherits tag that sets a config property or annotating service interfaces/etc, but it needs to be opt-in. If the server needs to behave differently depending on this setting, then either it should be included in the RPC payload or in some deploy artifact, such as the serialization policy file. -- John A. Tamplin Software Engineer (GWT), Google -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
Re: [gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
There is a risk that existing users were relying on current behavior to not send sensitive data in final fields across the wire Agreed, that is the worst case scenario. such as the serialization policy file. Yeah, my concern would be that the policy file is still somewhat optional, right? At least personally I end up occasionally using the legacy policy because I don't keep policy files across builds. I suppose we could use the policy file if available, otherwise default to the existing behavior. - Stephen -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
On 2011/09/01 00:10:12, zhuyi wrote: Hi Stephen, I've integrated your changes, also synced the changes. Sorry for the delay. Could you please review it? Thanks. Yi http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
http://gwt-code-reviews.appspot.com/1380807/diff/40002/user/src/com/google/gwt/user/RemoteServiceFinalFields.gwt.xml
File user/src/com/google/gwt/user/RemoteServiceFinalFields.gwt.xml
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/40002/user/src/com/google/gwt/user/RemoteServiceFinalFields.gwt.xml#newcode18
user/src/com/google/gwt/user/RemoteServiceFinalFields.gwt.xml:18: --
This comments seems like copy/paste from another gwt.xml file.
http://gwt-code-reviews.appspot.com/1380807/diff/40002/user/src/com/google/gwt/user/RemoteServiceFinalFieldsFalseNoWarn.gwt.xml
File
user/src/com/google/gwt/user/RemoteServiceFinalFieldsFalseNoWarn.gwt.xml
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/40002/user/src/com/google/gwt/user/RemoteServiceFinalFieldsFalseNoWarn.gwt.xml#newcode18
user/src/com/google/gwt/user/RemoteServiceFinalFieldsFalseNoWarn.gwt.xml:18:
--
Another comment that should be removed (or updated).
http://gwt-code-reviews.appspot.com/1380807/diff/40002/user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java
File
user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/40002/user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java#newcode617
user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java:617:
== Shared.SerializeFinalFieldsOptions.FALSE) {
I was really confused why this was == FALSE instead of != TRUE.
If the setting is FALSE and the field is not returned, then shouldn't
FALSE_NOWARN also mean that the field should be not returned? Seems
like they should be consistent.
It turns on this is broken--because you changed the server-side
signature to always include final fields, final fields always need to be
returned from this method.
(You correctly changed FieldSerializerCreator to re-check this setting
before writing out the field-setting code.)
It would be nice if FALSE|FALSE_NOWARN were set, then fields weren't
ever returned, but since the server-side type signature checker doesn't
have access to the type oracle settings to conditionally build its
signature, I don't see a better way.
Note that I found this by adding a FinalFieldsFalseTest--you add a
FinalFieldsTest and FinalFieldsFalseNoWarn (which worked because the
check above was == FALSE), but if you add a FinalFieldsFalseTest, the ==
FALSE check above kicks in, and then the signature check will fail.
http://gwt-code-reviews.appspot.com/1380807/diff/40002/user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java#newcode621
user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java:621:
* If the type has a custom seriaherelizer, assume the programmer knows
Looks like here got pasted into the middle serializer. :-)
http://gwt-code-reviews.appspot.com/1380807/diff/40002/user/src/com/google/gwt/user/rebind/rpc/Shared.java
File user/src/com/google/gwt/user/rebind/rpc/Shared.java (right):
http://gwt-code-reviews.appspot.com/1380807/diff/40002/user/src/com/google/gwt/user/rebind/rpc/Shared.java#newcode60
user/src/com/google/gwt/user/rebind/rpc/Shared.java:60: private static
SerializeFinalFieldsOptions serializeFinalFieldsValue;
This static field should be removed, it's not needed.
http://gwt-code-reviews.appspot.com/1380807/diff/40002/user/src/com/google/gwt/user/rebind/rpc/Shared.java#newcode108
user/src/com/google/gwt/user/rebind/rpc/Shared.java:108:
SerializeFinalFieldsOptions.valueOf(serializeFinalFieldsStringValue);
Just return the valueOf result instead of assigning it to the static
field, and then returning the static field.
http://gwt-code-reviews.appspot.com/1380807/diff/40002/user/src/com/google/gwt/user/rebind/rpc/Shared.java#newcode250
user/src/com/google/gwt/user/rebind/rpc/Shared.java:250: return propVal;
I would just return prop.getCurrentValue() instead of making an
otherwise unused propVal local variable.
http://gwt-code-reviews.appspot.com/1380807/diff/40002/user/src/com/google/gwt/user/server/rpc/impl/SerializabilityUtil.java
File
user/src/com/google/gwt/user/server/rpc/impl/SerializabilityUtil.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/40002/user/src/com/google/gwt/user/server/rpc/impl/SerializabilityUtil.java#newcode405
user/src/com/google/gwt/user/server/rpc/impl/SerializabilityUtil.java:405:
static boolean isNotStaticTransientOrEnum(Field field) {
The old isNonStaticTransientOrFinal method isn't called anymore so can
be removed since you added this one.
http://gwt-code-reviews.appspot.com/1380807/diff/40002/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestFalseNoWarn.java
File
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestFalseNoWarn.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/40002/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestFalseNoWarn.java#newcode27
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestFalseNoWarn.java:27:
public class
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
I've integrated your changes, also synced the changes. Thanks! Could you please review it? Sure. I understand a bit more about how it works now. Previously, the server-side ignored all final fields. You understandably had to change that, but since the server-side doesn't have access to the type oracle's rpc.final.serialize property, it *always* expects final fields. So, final fields are now always in the signatures, payloads, etc., and the FALSE FALSE_NOWARN behavior is implemented merely by skipping the setXxx call in FieldSerializerCreator. However, since that is only for the client-side, the server-side now always sets final fields, regardless of the rpc.final.serialize value. See this failing assertion: https://github.com/stephenh/scalagwt-gwt/commit/41abe191926ba31b7634f529f1ac8d455e729fe9#L3R52 Previously the server-side would have called the no-arg cstr (which assigns 5) and then done nothing else. So technically this is a breaking change. Is this intentional? That finals are always sent over the wire? It seems like making the server-side aware of the rpc.final.serialize value would be better, perhaps by putting it in the serialization policy file? Then it could know when it should/should not use final fields. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
I am integrating it. Sorry that I haven't worked on it for a while due to other jobs. On Fri, Aug 26, 2011 at 8:24 AM, stephen.haber...@gmail.com wrote: Thanks. I'll look through and incorporate your changes. zhuyi, any updates on this? http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
I am integrating it. Sorry that I haven't worked on it for a while due to other jobs. Awesome, no problem. Was just wondering if I should try and pick it up if you weren't working on it. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
Thanks. I'll look through and incorporate your changes. zhuyi, any updates on this? http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
On 2011/07/06 14:05:05, stephenh wrote: That would be great. Could you please upload the patch? Thanks! Sure--I can't upload a new patchset to this issue because I'm not the owner. Is it possible to add me? Or would you prefer for me to create a new issue? Could you please create a new issue? Sorry for the trouble. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
Could you please create a new issue? Yeah, np: http://gwt-code-reviews.appspot.com/1463813/ I'll reiterate my previous disclaimer that I didn't do any clean up for formatting/line-length/etc. as it was just a proof of concept. If you'd like me to clean it up to be directly submit-able, let me now. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
Thanks. I'll look through and incorporate your changes. On Thu, Jul 7, 2011 at 6:33 PM, stephen.haber...@gmail.com wrote: http://gwt-code-reviews.**appspot.com/1463813/http://gwt-code-reviews.appspot.com/1463813/ I deleted this issue and made a new one: http://gwt-code-reviews.**appspot.com/1463814/http://gwt-code-reviews.appspot.com/1463814/ That has better context between the last patch set of this issue and the changes I made to kill the static field. http://gwt-code-reviews.**appspot.com/1380807/http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
According to the current code structure, if we want to make the variable non-static, we may need to pass the propertyOracle parameter to many methods I downloaded the patch and was able to kill the flaky static variable by changing various methods to take GeneratorContextExt (which has both TypeOracle and PropertyOracle) instead of just TypeOracle. Changing existing method params from TypeOracle - GeneratorContextExt seemed like a nicer change than adding PropertyOracle as yet another method parameter, which, as you said, could get tedious. I was just seeing if it would work, so I didn't do any formatting/method ordering/etc., i.e. it's not perfect. But I can upload the patch if you'd like to see it. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
On 2011/07/06 06:22:38, stephenh wrote: According to the current code structure, if we want to make the variable non-static, we may need to pass the propertyOracle parameter to many methods I downloaded the patch and was able to kill the flaky static variable by changing various methods to take GeneratorContextExt (which has both TypeOracle and PropertyOracle) instead of just TypeOracle. Changing existing method params from TypeOracle - GeneratorContextExt seemed like a nicer change than adding PropertyOracle as yet another method parameter, which, as you said, could get tedious. I was just seeing if it would work, so I didn't do any formatting/method ordering/etc., i.e. it's not perfect. But I can upload the patch if you'd like to see it. That would be great. Could you please upload the patch? Thanks! http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
That would be great. Could you please upload the patch? Thanks! Sure--I can't upload a new patchset to this issue because I'm not the owner. Is it possible to add me? Or would you prefer for me to create a new issue? http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
On 2011/07/06 13:51:57, zhuyi wrote: On 2011/07/06 06:22:38, stephenh wrote: According to the current code structure, if we want to make the variable non-static, we may need to pass the propertyOracle parameter to many methods I downloaded the patch and was able to kill the flaky static variable by changing various methods to take GeneratorContextExt (which has both TypeOracle and PropertyOracle) instead of just TypeOracle. Changing existing method params from TypeOracle - GeneratorContextExt seemed like a nicer change than adding PropertyOracle as yet another method parameter, which, as you said, could get tedious. I was just seeing if it would work, so I didn't do any formatting/method ordering/etc., i.e. it's not perfect. But I can upload the patch if you'd like to see it. That would be great. Could you please upload the patch? Thanks! Ray and Bob: which way do you prefer? http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
Is this going to be submitted? Sounds like everything was fine, except for changing the test object back to IsSerializable? http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
On 2011/07/05 19:51:52, stephenh wrote: Is this going to be submitted? Sounds like everything was fine, except for changing the test object back to IsSerializable? Hi Bob and Ray, Any comments about my response below? I don't have the full context, but adding flakiness is not a good idea. According to the current code structure, if we want to make the variablenon-static, we may need to pass the propertyOracle parameter to many methods, because shouldConsiderFieldsForSerialization method in SerializableTypeOracleBuilder is static and many methods call this utility method. The change will make the code very cumbersome. I investigated the code couple of times but cannot figure out a better way. Please give some advice. Thanks. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
On 2011/05/09 21:06:01, zhuyi wrote:
On 2011/04/18 18:56:30, Stephen Chenney wrote:
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java
File
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java#newcode27
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java:27:
public
class FinalFieldsNode implements IsSerializable {
On 2011/04/18 17:39:35, zhuyi wrote:
On 2011/04/18 14:18:10, bobv wrote:
Use Serializable instead of IsSerializable.
Done.
I disagree. The legacy serialization policy requires IsSerializable,
and the
server JUnit tests use the legacy policy (because they do not set up
a
serialization whitelist file). To make this class useful in server
side tests,
I
would like it to be IsSerializable. I had to do that for some other
test class
so I could use it in my tests.
I think we should revert it back to IsSerializable for backward
compatibility?
SGTM.
Stephen.
http://gwt-code-reviews.appspot.com/1380807/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
I don't have the full context, but adding flakiness is not a good idea. According to the current code structure, if we want to make the variable non-static, we may need to pass the propertyOracle parameter to many methods, because shouldConsiderFieldsForSerialization method in SerializableTypeOracleBuilder is static and many methods call this utility method. The change will make the code very cumbersome. I investigated the code couple of times but cannot figure out a better way. Please give some advice. Thanks. http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/src/com/google/gwt/user/rebind/rpc/Shared.java
File user/src/com/google/gwt/user/rebind/rpc/Shared.java (right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/src/com/google/gwt/user/rebind/rpc/Shared.java#newcode56
user/src/com/google/gwt/user/rebind/rpc/Shared.java:56: private static
SerializeFinalFieldsOptions serializeFinalFieldsValue;
See comment in previous revision. Static fields in Generator types can
cause flaky behavior.
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/src/com/google/gwt/user/rebind/rpc/Shared.java#newcode100
user/src/com/google/gwt/user/rebind/rpc/Shared.java:100: propertyOracle,
RPC_PROP_SERIALIZE_FINAL_FIELDS, FALSE).toUpperCase();
You can use Enum.valueOf() instead of the if block below.
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java
File user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java#newcode56
user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java:56:
private FinalFieldsTestServiceAsync finalFieldsTestService;
Member sort order. Fields should be defined before methods.
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java
File
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java#newcode27
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java:27:
public class FinalFieldsNode implements IsSerializable {
Use Serializable instead of IsSerializable.
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/server/rpc/FinalFieldsTestServiceImpl.java
File
user/test/com/google/gwt/user/server/rpc/FinalFieldsTestServiceImpl.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/server/rpc/FinalFieldsTestServiceImpl.java#newcode28
user/test/com/google/gwt/user/server/rpc/FinalFieldsTestServiceImpl.java:28:
public FinalFieldsNode transferObject(FinalFieldsNode node) {
Check the incoming values in node to make sure that final fields are
being set properly when sent by the client.
http://gwt-code-reviews.appspot.com/1380807/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/src/com/google/gwt/user/rebind/rpc/Shared.java
File user/src/com/google/gwt/user/rebind/rpc/Shared.java (right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/src/com/google/gwt/user/rebind/rpc/Shared.java#newcode56
user/src/com/google/gwt/user/rebind/rpc/Shared.java:56: private static
SerializeFinalFieldsOptions serializeFinalFieldsValue;
On 2011/04/18 14:18:10, bobv wrote:
See comment in previous revision. Static fields in Generator types
can cause
flaky behavior.
Agree. However, if I make the value non-static, then a lot of changes
should be made in the legacy. We may keep it static for now.
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/src/com/google/gwt/user/rebind/rpc/Shared.java#newcode100
user/src/com/google/gwt/user/rebind/rpc/Shared.java:100: propertyOracle,
RPC_PROP_SERIALIZE_FINAL_FIELDS, FALSE).toUpperCase();
On 2011/04/18 14:18:10, bobv wrote:
You can use Enum.valueOf() instead of the if block below.
Done.
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java
File user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java#newcode56
user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java:56:
private FinalFieldsTestServiceAsync finalFieldsTestService;
On 2011/04/18 14:18:10, bobv wrote:
Member sort order. Fields should be defined before methods.
Done. Also did in FinalFieldsTestFalseNoWarn.java
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java
File
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java#newcode27
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java:27:
public class FinalFieldsNode implements IsSerializable {
On 2011/04/18 14:18:10, bobv wrote:
Use Serializable instead of IsSerializable.
Done.
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/server/rpc/FinalFieldsTestServiceImpl.java
File
user/test/com/google/gwt/user/server/rpc/FinalFieldsTestServiceImpl.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/server/rpc/FinalFieldsTestServiceImpl.java#newcode28
user/test/com/google/gwt/user/server/rpc/FinalFieldsTestServiceImpl.java:28:
public FinalFieldsNode transferObject(FinalFieldsNode node) {
On 2011/04/18 14:18:10, bobv wrote:
Check the incoming values in node to make sure that final fields are
being set
properly when sent by the client.
Done.
http://gwt-code-reviews.appspot.com/1380807/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
On Mon, Apr 18, 2011 at 10:39 AM, zh...@google.com wrote:
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/src/com/google/gwt/user/rebind/rpc/Shared.java
File user/src/com/google/gwt/user/rebind/rpc/Shared.java (right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/src/com/google/gwt/user/rebind/rpc/Shared.java#newcode56
user/src/com/google/gwt/user/rebind/rpc/Shared.java:56: private static
SerializeFinalFieldsOptions serializeFinalFieldsValue;
On 2011/04/18 14:18:10, bobv wrote:
See comment in previous revision. Static fields in Generator types
can cause
flaky behavior.
Agree. However, if I make the value non-static, then a lot of changes
should be made in the legacy. We may keep it static for now.
I don't have the full context, but adding flakiness is not a good idea.
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/src/com/google/gwt/user/rebind/rpc/Shared.java#newcode100
user/src/com/google/gwt/user/rebind/rpc/Shared.java:100: propertyOracle,
RPC_PROP_SERIALIZE_FINAL_FIELDS, FALSE).toUpperCase();
On 2011/04/18 14:18:10, bobv wrote:
You can use Enum.valueOf() instead of the if block below.
Done.
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java
File user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java#newcode56
user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java:56:
private FinalFieldsTestServiceAsync finalFieldsTestService;
On 2011/04/18 14:18:10, bobv wrote:
Member sort order. Fields should be defined before methods.
Done. Also did in FinalFieldsTestFalseNoWarn.java
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java
File
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java#newcode27
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java:27:
public class FinalFieldsNode implements IsSerializable {
On 2011/04/18 14:18:10, bobv wrote:
Use Serializable instead of IsSerializable.
Done.
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/server/rpc/FinalFieldsTestServiceImpl.java
File
user/test/com/google/gwt/user/server/rpc/FinalFieldsTestServiceImpl.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/server/rpc/FinalFieldsTestServiceImpl.java#newcode28
user/test/com/google/gwt/user/server/rpc/FinalFieldsTestServiceImpl.java:28:
public FinalFieldsNode transferObject(FinalFieldsNode node) {
On 2011/04/18 14:18:10, bobv wrote:
Check the incoming values in node to make sure that final fields are
being set
properly when sent by the client.
Done.
http://gwt-code-reviews.appspot.com/1380807/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java
File
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/11003/user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java#newcode27
user/test/com/google/gwt/user/client/rpc/FinalFieldsTestService.java:27:
public class FinalFieldsNode implements IsSerializable {
On 2011/04/18 17:39:35, zhuyi wrote:
On 2011/04/18 14:18:10, bobv wrote:
Use Serializable instead of IsSerializable.
Done.
I disagree. The legacy serialization policy requires IsSerializable, and
the server JUnit tests use the legacy policy (because they do not set up
a serialization whitelist file). To make this class useful in server
side tests, I would like it to be IsSerializable. I had to do that for
some other test class so I could use it in my tests.
http://gwt-code-reviews.appspot.com/1380807/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
Ping. Is this going to be submitted? http://gwt-code-reviews.appspot.com/1380807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Serialization of Final Fields in RPC (issue1380807)
http://gwt-code-reviews.appspot.com/1380807/diff/4001/user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java
File
user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/4001/user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java#newcode617
user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java:617:
}
Extra whitespace.
http://gwt-code-reviews.appspot.com/1380807/diff/4001/user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java#newcode742
user/src/com/google/gwt/user/rebind/rpc/SerializableTypeOracleBuilder.java:742:
private final Shared.SerializeFinalFieldsOptions
shouldSerializeFinalFields;
Sort fields alphabetically unless they have an initialization
dependency. While you're at it, remove the blank lines within the field
declarations.
http://gwt-code-reviews.appspot.com/1380807/diff/4001/user/src/com/google/gwt/user/rebind/rpc/Shared.java
File user/src/com/google/gwt/user/rebind/rpc/Shared.java (right):
http://gwt-code-reviews.appspot.com/1380807/diff/4001/user/src/com/google/gwt/user/rebind/rpc/Shared.java#newcode56
user/src/com/google/gwt/user/rebind/rpc/Shared.java:56: private static
SerializeFinalFieldsOptions serializeFinalFieldsValue;
Don't store this in a static field, since it's dependent on the values
of a PropertyOracle.
http://gwt-code-reviews.appspot.com/1380807/diff/4001/user/src/com/google/gwt/user/server/rpc/impl/SerializabilityUtil.java
File
user/src/com/google/gwt/user/server/rpc/impl/SerializabilityUtil.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/4001/user/src/com/google/gwt/user/server/rpc/impl/SerializabilityUtil.java#newcode279
user/src/com/google/gwt/user/server/rpc/impl/SerializabilityUtil.java:279:
static boolean isNotStaticTransientOrFinal(Field field) {
Is this method still used?
http://gwt-code-reviews.appspot.com/1380807/diff/4001/user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamReader.java
File
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamReader.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/4001/user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamReader.java#newcode686
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamReader.java:686:
!Modifier.isPublic(declField.getModifiers())) ||
Modifier.isFinal(declField.getModifiers());
Double-check formatting here.
http://gwt-code-reviews.appspot.com/1380807/diff/4001/user/test/com/google/gwt/user/RPCFinalFieldsTest.gwt.xml
File user/test/com/google/gwt/user/RPCFinalFieldsTest.gwt.xml (right):
http://gwt-code-reviews.appspot.com/1380807/diff/4001/user/test/com/google/gwt/user/RPCFinalFieldsTest.gwt.xml#newcode20
user/test/com/google/gwt/user/RPCFinalFieldsTest.gwt.xml:20:
set-property name='rpc.enforceTypeVersioning' value='true' /
What is this property for?
http://gwt-code-reviews.appspot.com/1380807/diff/4001/user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java
File user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java
(right):
http://gwt-code-reviews.appspot.com/1380807/diff/4001/user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java#newcode22
user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java:22:
Extra blank lines here and in other test code.
http://gwt-code-reviews.appspot.com/1380807/diff/4001/user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java#newcode33
user/test/com/google/gwt/user/client/rpc/FinalFieldsTest.java:33:
FinalFieldsNode node = new FinalFieldsNode();
Is there a test anywhere of final field values being sent from the
client? This test uses the default constructor, which is also called by
the server code. Instead, shouldn't this pass non-default values to the
three-arg constructor to verify that the server code properly resets
final values?
http://gwt-code-reviews.appspot.com/1380807/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
