Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Store UserOptions in DbConnectionPool.PendingGetConnection#40342

Closed
stephentoub wants to merge 1 commit into
dotnet:masterfrom
stephentoub:useroptions
Closed

Store UserOptions in DbConnectionPool.PendingGetConnection#40342
stephentoub wants to merge 1 commit into
dotnet:masterfrom
stephentoub:useroptions

Conversation

@stephentoub
Copy link
Copy Markdown
Member

@roji, is this worth fixing? It does seem like these were supposed to be stored such that they'd be passed through to subsequent operations, but this code has also been around for a very long time.

@roji
Copy link
Copy Markdown
Member

roji commented Aug 15, 2019

This actually has people who know what's going on, /cc @David-Engel @maryamariyan @Wraith2. It definitely does look like this could be a bug.

@divega
Copy link
Copy Markdown

divega commented Aug 16, 2019

Also cc @cheenamalhotra. Microsoft.Data.SqlClient likely has a copy of the same code with the same potential bug.

@cheenamalhotra
Copy link
Copy Markdown
Member

Thanks @divega

I have ported over this change to Microsoft.Data.SqlClient. Strangely this issue also existed in NetFx source code, seems like a miss from long time.

@divega
Copy link
Copy Markdown

divega commented Aug 16, 2019

It seems to exist on every copy of the code (there a separate copy for each provider) in this repo. I think we need someone to dive a bit deeper to understand the ramifications of both the original (apparently buggy) code and of taking this change.

@cheenamalhotra
Copy link
Copy Markdown
Member

cheenamalhotra commented Aug 16, 2019

I looked into the property usage and seems like this PR is not going to add any value, the userOptions property is passed all the way to DbConnectionFactory:L372 after which it disappears.

Implementation in SqlClient also treats it as null: SqlConnectionFactory:L31 and SqlConnectionFactory:L34

@stephentoub
Copy link
Copy Markdown
Member Author

Ok. I'll close this... but if it's actually not going to add value, it would seem instead there's some dead code here to be deleted.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants