Serialize final fields in GWT-RPC

[dankurka]

Originally reported on Google Code with ID 1054

Found in GWT Release:
All

Detailed description:
The current implementation of GWT RPC does not serialize final fields which hinders
the ability to 
use explicitly immutable objects.  The only obstacle to solving this problem is the
server side 
deserialization code.  The reason is that  the act of changing the value of a final
field via 
reflection is not clearly specified and is likely to be a JVM implementation detail.
 For example, 
the JRE itself uses unsafe methods to actually achieve this.

The solutions range from simply trying the reflective path to see if it works to actually
doing 
some form of GWT stream to Java serialized stream conversion and then using Java 
deserialization logic to handle the rest.  

Workaround if you have one:

Links to the relevant GWT Developer Forum posts:

Reported by gwt.team.mmendez on 2007-05-17 15:00:05

[dankurka]

Carlos,

Vaadin is based on GWT, the same limitation applies.

Sebastiaan

Reported by sebastiaan.blommers on 2013-06-03 13:51:48

[dankurka]

@fry, no real update on this, although I would still like to get it in.

I might ask on the contrib list about just dropping the config variables and making
final fields always get serialized. It makes more sense and would make the code simpler.

Reported by stephen.haberman on 2013-06-03 14:46:43

[dankurka]

With a huge dufference - vaadin works on the server side. On the client
site you have GWT widgets and events, that's all. No need to serialize all
your model domains to pass data back and forth.

Reported by carlos.mfa.martins on 2013-06-03 21:32:44

[dankurka]

@Brian, @Stephen: Are we going to include this in the 2.6 release?

Reported by goktug@google.com on 2013-10-01 06:03:34

[dankurka]

So was this included in 2.6 or not ?

Reported by david.nouls on 2014-02-19 09:47:50

[dankurka]

No, it was not in 2.6.

Reported by stephen.haberman on 2014-02-19 16:06:43

[dankurka]

Would it be possible to at least include final types in the whitelist? Currently, CustomFieldSerializers
are essentially useless as they invariably fail with a whitelist error.

Reported by chrispurcell@google.com on 2014-05-20 16:06:18

[dankurka]

That sounds like it might be a bug but I'm not quite sure what you mean. Could you create
a new bug with a specific example?

Reported by skybrian@google.com on 2014-05-20 16:32:13

[dankurka]

https://code.google.com/p/google-web-toolkit/issues/detail?id=8728

Reported by chrispurcell@google.com on 2014-05-21 11:03:09

[dankurka]

GWT 2.6.1

Serializing final fields still doesn't work in GWT 2.6.1. Also the property "rpc.final.serialize"
doesn't exists:

<set-property name="rpc.final.serialize" value="true" />

Is there a chance to release this with 2.7.0?

Reported by dejavue88 on 2014-09-10 10:12:40

[dankurka]

It's unlikely at this point. This patch is stalled on migration issues and we are busy
with other features for 2.7.

Reported by skybrian@google.com on 2014-09-10 19:08:44

[dankurka]

@Brian, @Stephen: Is there any blocking issue to make this available as 'opt-in' for
2.7 release?

Reported by goktug@google.com on 2014-09-10 19:15:25

[dankurka]

It would result in people running GWT-RPC in a mode that's mostly untested and perhaps
might be dropped if we figure out a better way to do it. It took us a while to drop
deRPC so I don't think we should add new modes like this. If we do it, we should do
a proper migration like we are with GSS.

Reported by skybrian@google.com on 2014-09-10 19:24:14

[dankurka]

We don't have a big investment on GWT-RPC and people are waiting for this for very long
time so I think we shouldn't keep them waiting for something that we are not even working.
This is not like deRPC or GSS where we had a separate code base to replace existing
one. This is a few hundred lines of code, it is not replacing an existing system and
it is ok to be opt-in forever.
If we are not confident and need more testing then we can release it with 'experimental'
name on the flag, otherwise we will never get there.

Reported by goktug@google.com on 2014-09-10 19:45:55

[dankurka]

I don't think we can wish away the migration issues. We have a lot of code that uses
GWT-RPC and it affects teams like Guava who support GWT-RPC for their types.

If some of the people using Guava start using this flag and Guava doesn't work right
with it on, should they start supporting GWT-RPC both with and without the flag turned
on? I don't want to explain to library maintainers that they have to have to worry
about two modes of operation for GWT-RPC; they have their hands full already with Java
8, etc. Not using "final" for a field is an easy enough workaround and we've been getting
by for years, so this doesn't seem urgent.

Reported by skybrian@google.com on 2014-09-10 21:41:24

[dankurka]

Awww.  Pretty please?  This does really horrible things to our code, it's difficult
to even reason about when things can't be final.  (We have 100+ complex DTO's built
up over many years)

What about an annotation

   @GwtSerializedFinal final ArrayList myList = new ArrayList();

?

Reported by pmclachlan on 2014-09-10 22:28:10

[dankurka]

> This does really horrible things to our code, it's difficult
> to even reason about when things can't be final.

I'm also a huge proponent of immutable value objects, but if you need immutability
to make it easier to reason about your code, a workaround is to make the class "effectively
immutable", without using final fields:

@Immutable
@ParametersAreNonNullByDefault
public class Money {

  private /*gwtfinal*/ BigDecimal amount;
  private /*gwtfinal*/ Currency currency;

  @NoArgConstructorForGwt
  private Money() {}

  public Money(BigDecimal amount, Currency currency) {
    this.amount = checkNotNull(amount);
    this.currency = checkNotNull(currency);
  }

  public BigDecimal getAmount() {
    return amount;
  }

  public Currency getCurrency() {
    return currency;
  }

}

@NoArgConstructorForGwt is just a compile-time annotation to document why we need a
no-arg constructor (its Javadoc explains that it's needed for GWT, that the constructor
can be private, etc.).
The /* gwtfinal */ comment is there to signify that it should be final, but cannot
due to GWT-RPC serialization. When final fields will be serializable, it will be easy
to search-and-replace those comments.

Anyway, that's what I do in my current project. Of course, it needs boilerplate code,
and it's not as pretty as "real" final fields. You also don't get any performance benefits...
But it's still better than nothing.

Reported by neveue on 2014-09-10 22:42:37

[dankurka]

Not using final is an "annoying" work around, that's why this is in the top 5 most voted
issues in GWT for years.

Besides, Guava and others already have the custom field serializers when they have
final fields basically because we don't support it so they are not affected by it.
Even, there is even a corner case where they are affected by it, we can just add the
transient / @GwtTransient to the field.

And again if I'm missing something and it becomes much more complicated than that,
Guava doesn't need to support it until this comes out of experimentation or becomes
the default.

Reported by goktug@google.com on 2014-09-10 22:48:50

[dankurka]

Personally, I think we should just add the option, let users turn it on if it works
for them, and leave it off and file new bugs if it does not.

Brian has found a large app within Google where walking final fields (which were previously
skipped) leads to now pulling in java.lang.Object. Which sucks for that app. But, fine,
that app (and all Google apps for all I care) can just leave the setting off.

Tangentially, the current version of the patch has a semi-wonky audit value, that was
to address some of Brian's concerns. It either needs touched up, or just removed for
now. We can always add it back later.

That said, I have very little zero motivation to work on this, as I don't have any
confidence the patch will end up getting +1'd.

Reported by stephen.haberman on 2014-09-10 23:03:22

[dankurka]

I have code where I use public final fields because I want to express to users of the
class that the value will never change. Making those fields non-final would make this
very dangerous. Wrapping those fields in accessors doesn't give client code the confidence
that it will always get the same instance.

'final' is more than just an optimization but an important feature of the language.
Therefore not serializing final fields is definitely a serious bug and not just an
ill-advised feature. Therefore a possible fix might change wildly in implementation
in the future but not in effect.

I say put it in and add a flag for backwards compatibility. In 3.0 the new behaviour
should become default. 

Regarding the concerns about CustomFieldSerializers in the patchset comments: the computation
of type reachability and serializability for CustomFieldSerializers is broken and needs
fixing but there are workarounds that so that should not be a reason to delay this
fix.

Please don't let the perfect prevent the good enough.

Reported by guus@bloemsma.net on 2014-09-11 04:43:24

[dankurka]

I agree with Goktug, that not using final is an "annoying" work around. I think GWT-RPC
is a great technology, but changing the domain model for using it with an specific
technology is a bad solution. My dream is to use one bean with "all" important technologies
without making technology-specific fittings on it. Supporting final fields in GWT-RPC
will get a little closer to fulfilling this dream.

This issue is 7 years old and in the top 5 most voted issues in GWT. What can be more
important than fix this?

So please fix it! +10

Regards

Tino

Reported by dejavue88 on 2014-09-11 09:24:15

[dankurka]

Let's try to get a sense of perspective. This cannot possibly be the most important
issue to fix in GWT since it has an easy workaround and we've lived with it for years.
The reason it has lots of stars is that it's old. Older bugs always have more stars
since they're widely known and have been around longer.

But anyway, I'm not saying we shouldn't do it, just that we shouldn't do it for 2.7,
since we're close to the release date and we're working other things like Super Dev
Mode performance and usability and GSS (hopefully). 

Regarding migration, when I first brought it up it was dismissed as a theoretical issue
and then when I found an example it's supposedly an outlier. However, the breakage
was testing a change that doesn't even fully enable final fields; it only starts visiting
them for generators. Also, I only noticed the issue because it actually broke something;
other apps could be affected by code bloat and I wouldn't have noticed if it didn't
actually break.

So my position is that when I took the time to look, I found one clear breakage and
there are probably more. It's also made me suspicious about code bloat; would like
to see how much these changes increase the size of downloadeded GWT apps by adding
dependencies on unneeded code. That will take a bit of work and other things have been
higher priority.

As we found out when we tried to make changes that require cleaning up module files
and CSS files, there is a lot of cruft out there. In widely used code with a complicated
interface, things are often working by coincidence and ignoring migration issues will
make a mess. This is true of GWT-RPC as well. Unfortunately this makes progress harder
than we'd like.

Reported by skybrian@google.com on 2014-09-11 20:08:14

[dankurka]

This bug affected me a few years ago, and I'm kind of surprised it's still
around. The bug was a big surprise to me and caused me to waste a lot of
time tracking it down.

Steven Sudit | Knowledge Panels | sudit@google.com | 917-406-7883
I welcome *VSRE* emails. Learn more at http://vsre.info/

Reported by sudit@google.com on 2014-09-11 20:10:08

[dankurka]

> clear breakage and there are probably mor

Right, but the breakage was not due to the final field functionality itself, it was
due to the app having bad GWT-RPC dependencies pulled in (as you said, pulling in java.lang.Exception
and all subclasses).

Which is a breakage *for that app*. But they can just keep final fields off.

Have you found any breakages with the functionality of this flag itself, given a sane/smaller/typical
GWT app?

At this point, I don't really care if/when any Google apps run with final fields=true.
I understand you face a lot of complexities internally. That's fine. That is your lot
in life. :-)

But there are plenty of us outside that would like to try it, and if it works for our
apps, why deny us this option?

Reported by stephen.haberman on 2014-09-11 21:16:22

[dankurka]

I don't want to split the community between people who turn on this feature and people
who leave it off. Then widely used libraries have to support both modes without knowing
whether their downstream dependencies turned it on or not.

A whitelist or blacklist of types would be a way to avoid this. Much as I hate to add
configuration properties, that might be the way to go.

Reported by skybrian@google.com on 2014-09-11 21:28:11

[dankurka]

We don't need to make this complicated. As I told a few comments earlier:

Guava and others doesn't need to support two modes.
They already have custom field serializers if they have final fields, otherwise it
won't work. Those types will not be affected by the flag.

The only and probably unlikely scenario is, they might be relying on GWT to not send
the final fields before without marking them transient or GwtTransient.
If they are in that situation, this is already a warning in GWT-RPC, and they should
add the transient marker regardless of this change.

Reported by goktug@google.com on 2014-09-11 21:54:59

[dankurka]

Actually we already have whitelisting/blacklisting support in GWT-RPC. If somebody ends
up needing it, that's already available.

Reported by goktug@google.com on 2014-09-11 22:00:04

[dankurka]

https://gwt-review.googlesource.com/9683

Reported by goktug@google.com on 2014-10-15 20:49:44

• Labels added: Milestone-2_7

[dankurka]

Reported by goktug@google.com on 2014-10-19 22:18:54

• Status changed: FixedNotReleased

[dankurka]

Wow, thats great!

Reported by dejavue88 on 2014-10-20 09:51:25

[dankurka]

Sorry, this is not really fixed yet.

We have an experimental flag that you will be able to try in 2.7, but we don't guarantee
that it will work for everyone. Libraries shouldn't assume final fields work yet because
not everyone will turn it on.

More work needs to be done on migrating existing apps before we can make this the default.

Reported by skybrian@google.com on 2014-10-20 17:08:28

• Status changed: Started
• Labels removed: Milestone-2_7

[dankurka]

Csak vicc volt

Reported by pihentagy on 2014-10-20 19:37:09

[dankurka]

Unlikely that this will get addressed since we are deprecating GWT RPC since it can not easily be ported over to j2cl.