[Event Hub] Small improvements & refactorings

[danielmarbach]

Contributing to the Azure SDK

Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.

For specific information about pull request etiquette and best practices, see this section.

[ghost]

Thank you for your contribution @danielmarbach! We will review the pull request and get back to you soon.

[danielmarbach]

@jsquire I noticed GetPooledProducer might call TransportProducerFactory multiple times because GetOrAdd might call the value factory multiple times. Maybe it is not a problem I just made me suspicious because the transport producer is a resource that needs to be closed if I understand the code correctly.

[danielmarbach]

Please also see #29952 (comment)

[danielmarbach]

Ah I think these comments

// A race condition at this point may end with CloseAsync called on
            // the returned PoolItem if it had expired. The probability is very low and
            // possible exceptions should be handled by the invoking methods.

// If TryRemove returned false the PoolItem would not be closed deterministically
                // and the ExpirationTimer callback would eventually remove it from the
                // Pool leaving to the Garbage Collector the responsibility of closing
                // the TransportProducer and the AMQP link.

explains things

[danielmarbach]

I have another design related question. I wonder if it would make sense to add the necessary state and references to PooledProducer and then make the DisposeAsync method do the cleaning. With that the pooled producer could have a reference to the PooledItem and piggy back the identifier and what is needed for the cleanup. While this class is a bit sneaky how it reaches into the parent state it would get rid of the unnecessary closure.

[jsquire]

I have another design related question. I wonder if it would make sense to add the necessary state and references to PooledProducer and then make the DisposeAsync method do the cleaning. With that the pooled producer could have a reference to the PooledItem and piggy back the identifier and what is needed for the cleanup. While this class is a bit sneaky how it reaches into the parent state it would get rid of the unnecessary closure.

I generally have a strong preference to keep object graphs flowing in a single direction, but this is an internal construct of very tightly coupled types, so I'm not totally opposed in this specific instance. I do know that we originally ran into a lot of issues with concurrent use and non-deterministic clean-up that we had to guard against. (the two comments that you highlighted call some out)

Do you think we'd be opening up additional corner cases to guard against?

[danielmarbach]

I guess this is good to go then

[jsquire]

I guess this is good to go then

Much appreciated!