HTTP2: Fix hashCode() and equals(...) implementation #13903
Conversation
Motivation: Our implementations of hashCode() and equals(...) were not correct for some frames. Modifications: Correctly implement both methods Result: Fixes #13659
There was a problem hiding this comment.
In general, I worry about properly implementing hashCode() for objects with mutable state since it can lead to hash table corruption. The risk feels more pronounced in DefaultHttp2FrameStream as its fields are volatile and set in couple different places.
Is that the intended use case of these methods? Most of them don't feel like good candidates for keys in a map/set as it is. .equals(..) is more valuable especially for tests. If that's the primary use case we could go with naive hashCode methods despite the high probability of collision.
| DefaultHttp2FrameStream that = (DefaultHttp2FrameStream) o; | ||
| Http2Stream stream = this.stream; | ||
| Http2Stream thatStream = that.stream; | ||
| return id == that.id && stream != null && stream.equals(thatStream); |
There was a problem hiding this comment.
Are two DefaultHttp2FrameStream considered equal if this.stream == that.stream == null?
There was a problem hiding this comment.
I would say yes, but it's a good point @bryce-anderson (maybe I'm wrong!)
There was a problem hiding this comment.
I think they'd be considered equal as well, although I don't believe it's covered by this implementation.
This would probably do it:
return this.id == that.id && Objects.equals(this.stream, that.stream);
|
How did you discovered @normanmaurer ? |
|
@franz1981 someone else did #13659 |
Motivation:
Our implementations of hashCode() and equals(...) were not correct for some frames.
Modifications:
Correctly implement both methods
Result:
Fixes #13659