GH-3242: Key StreamBridge function cache by binding name - #3244
Conversation
HDPark95
left a comment
There was a problem hiding this comment.
The binding-name source change fixes the reported 7/1 case, but the cache is still keyed by a single int, so the new code cannot provide the "must never share" guarantee in the comment.
I reproduced a remaining collision on this head with two accepted binding configurations:
partitioned dynamic binding: name="A>", partitionCount=120, partitionKeyExtractorName set
non-partitioned binding: name="nonPartitioned-out-0", partitionCount=1
Objects.hash("application/json", false, true, 120, "A>") = 682613285
Objects.hash("application/json", false, false, 1, null) = 682613285
The first send to A> succeeds and carries scst_partition. The following send to nonPartitioned-out-0 then reuses that cached wrapper and fails at PartitionHandler.extractKey with the same IllegalArgumentException: Partition key cannot be null reported in #3242. The focused StreamBridgeTests probe is 1 error on the submitted code.
Could the cache use a value key (for example, a record containing these five fields) instead of storing only the computed hash? ConcurrentHashMap would then use equality to distinguish colliding hashes. I verified that direction locally against the same probe: it becomes 1 test / 0 failures, and the existing test_2783 cache-sharing assertion remains 1 test / 0 failures.
StreamBridge caches the FunctionInvocationWrapper used by send(..) under the producer properties. The binding name only took part in that key when partitionKeyExpression and ProducerProperties#getBindingName() were both set, and getBindingName() is never populated on the instance StreamBridge reads: BindingService#bindProducer populates it on the extended copy it creates for an ExtendedPropertiesBinder, not on the original returned by BindingServiceProperties#getProducerProperties. A partitioned binding therefore shared its cached function with another binding, and since the partition enhancer is left on the cached function after a send that produced a partition header, the next send on the other binding failed with IllegalArgumentException: Partition key cannot be null. Take the binding name from the send(..) argument, which is always available, and include it for partitioned bindings. Key the cache by a record of the five properties rather than by their computed int hash, so bindings are told apart by equality and no hash collision can make two of them share a function. Bindings that are not partitioned keep sharing a cached function as before. Signed-off-by: kdelay <kdelay20@gmail.com>
25294be to
76bf8f0
Compare
|
Confirmed on 25294be: Done in 76bf8f0. The cache now uses a value key: I also rebased onto main (a2bbc43) since the branch had fallen behind. Verified on 76bf8f0:
|
Fixes #3242
What
StreamBridge.send(..)caches theFunctionInvocationWrapperunder a hash of the producer properties. The binding name only entered that hash whenpartitionKeyExpressionandProducerProperties#getBindingName()were both non-null, andgetBindingName()is never populated on the instanceStreamBridgereads:BindingService#bindProducercallspopulateBindingName(..)on the extended copy it builds for anExtendedPropertiesBinder, whileStreamBridgereads the original fromBindingServiceProperties#getProducerProperties(bindingName).So a partitioned binding can share its cached function with another binding.
PartitionAwareFunctionWrappersets the partition enhancer on that shared function and only clears it when the result carries noscst_partitionheader, so after a successful partitioned send the enhancer stays on the cached function. The next send on the colliding binding then runs throughPartitionHandlerwithout a partition key and fails withIllegalArgumentException: Partition key cannot be null.The hash also summed its components, so
Boolean.hashCode(true) + partitionCountandBoolean.hashCode(false) + partitionCountcollide whenever the partitioned binding's count is 6 higher than the other one's.Change
The cache is keyed by a record of
outputContentType,useNativeEncoding,partitioned,partitionCountandbindingNameinstead of by a computedint, soConcurrentHashMapdistinguishes bindings by equality and no hash collision can make two of them share a function. The binding name comes from thesend(..)argument, which is always available, and is only part of the key for partitioned bindings, so non-partitioned bindings keep sharing one cached function and the behaviour asserted bytest_2783is unchanged.Verification
test_3242inStreamBridgeTestsreproduces the failure: a binding partitioned viapartition-key-extractor-name(which leavespartitionKeyExpressionnull, so the old guard never added the binding name) withpartition-count: 7, and a non-partitioned binding withpartition-count: 1. Onmainthe second send fails withPartition key cannot be null; with this change both sends succeed and only the partitioned message carriesscst_partition.partitionedBindingIsNotSharedWithHashCollidingBindingcovers the collision case: two accepted configurations whose properties hash alike underObjects.hash(..)(both682613285). A cache keyed by that hash hands the partition-aware function to the non-partitioned send, which then fails the same way; with the value key both sends succeed and the cache holds two entries../mvnw -f core/pom.xml clean install(the build CI runs): BUILD SUCCESS.StreamBridgeTests: 41 tests, 1 failure. The failure istest_3033, which fails the same way onmainat a2bbc43, before this change (that module is currently commented out of thecorereactor).spring-cloud-stream-integration-testsmodule, this branch versus its base a2bbc43: 13 tests fail here and 14 on the base, and the set here is a subset of the base's. The one extra failure on the base (MultipleInputOutputFunctionTests.multiInputSingleOutputWithCustomContentType2) passes when run on its own, so it is flaky in the full run.