Azure / azure-storage-java

Microsoft Azure Storage Library for Java
https://docs.microsoft.com/en-us/java/api/overview/azure/storage
MIT License
190 stars 164 forks source link

Fix for keepalivesocketfactory regression #542

Closed mariosmeim-db closed 4 years ago

mariosmeim-db commented 4 years ago

This PR introduces the following changes:

One possible issue is thread-safety of the KeepAliveSocketFactory class. Specifically, do you think we should take further steps to ensure that the createSocket methods are thread safe? Any other concerns?

msftclas commented 4 years ago

CLA assistant check
All CLA requirements met.

rickle-msft commented 4 years ago

@mariosmeim-db Thank you for submitting this! We appreciate the contribution.

I'll actually ask you to keep the default unchanged just in case there are other unforseen behavior changes. I know you guys have done testing, but it's actually the policy of my team to not change the default with these sorts of things to preserve behavior for existing customers. When I changed the default in 8.6.1, that was my mistake.

I don't have any concerns about thread-safety, though it is a good question. There's no state held in our wrapper implementation of the factory, so there's not really an opportunity for threads to corrupt the state of the factory. I think the underlying implementation will have taken any necessary precautions for thread-safety as this type was probably designed to be used in this way (setting it on a connection). And the only state we are changing, we always set to true, so even if threads do interfere with each other (which I again don't think is possible as all the logic is local to the method), they'll all be setting the same value anyways. Is there something else I missed that you might be concerned about?

Please note that you need to sign the CLA before this can be merged.