io.grpc.Attributes - reference vs. object equality

[nicktrav]

io.grpc.Attributes was modified in 291f170 to use an IdentityMap to store the underlying attributes. This map uses reference equality in place of the object equality that was used in prior versions, when the class used HashMap.

This is more of a question around the expected behavior of Attributes#equals and the notion of "attribute equality" as opposed to a bug or an issue. For context, we have some tests internally that are failing in 1.8 as they were written with object equality in mind.

For example, the following fails in 1.8 but passes in prior versions, which upon immediate inspection wasn't intuitive, as InetSocketAddress#equals returns true when comparing the two different objects:

  @Test
  public void equals() {
    Attributes.Key<InetSocketAddress> attr = Attributes.Key.of("foo");

    InetSocketAddress addr1 = new InetSocketAddress(80);
    Attributes attrs1 = Attributes.newBuilder()
        .set(attr, addr1)
        .build();

    InetSocketAddress addr2 = new InetSocketAddress(80);
    Attributes attrs2 = Attributes.newBuilder()
        .set(attr, addr2)
        .build();

    assertEquals(addr1, addr2);   // passes
    assertEquals(attrs1, attrs2); // fails
  }

I'm interested in the motivation for the change in semantics (talking with @lukaszx0 in person, he recalls some talk about this previously). If reference equality was the intention, perhaps we could tighten up the javadoc a little to make it more explicit that this class now uses reference equality as opposed to object equality?

Or, perhaps this wasn't the original intention and it would be possible to revert to using a HashMap, although the benchmarks seem to indicate this would be slightly slower than the current impl.

[carl-mastrangelo]

I don't believe that side effect was intended, only the keys were supposed to be compared by reference equality. We could change it back, because it seems like a legit use case.

Do you mainly use it in tests or in prod code?

[ejona86]

We should change it back. Yeah, we should use object-equality for the values themselves.

[nicktrav]

@carl-mastrangelo @ejona86 - from what I can tell we're only relying on this in tests right now.

Thanks for the prompt reply / fix nonetheless! 🙏

[nicktrav]

(ignore my trigger finger on the close button)