Skip to content

Make type handlers thread-safe#4684

Merged
roji merged 1 commit intonpgsql:mainfrom
roji:ConcurrentHandlers
Oct 3, 2022
Merged

Make type handlers thread-safe#4684
roji merged 1 commit intonpgsql:mainfrom
roji:ConcurrentHandlers

Conversation

@roji
Copy link
Member

@roji roji commented Sep 28, 2022

@roji roji force-pushed the ConcurrentHandlers branch from b560e6b to b757abd Compare September 28, 2022 21:50
@roji roji force-pushed the ConcurrentHandlers branch from b757abd to f00bca2 Compare September 29, 2022 10:12
@roji
Copy link
Member Author

roji commented Sep 29, 2022

Note: I've added a concurrency test for the NetTopologySuite plugin, which seems to show that it's fine to use the reader/writer streams concurrently. So I think our handlers are now fine.

Note that I'm still not 100% convinced it's a good idea to share type handlers. The reason for doing this is mainly multiplexing: we want to bind parameters to the type handler, and do parameter validation/length calculation before we push the command into the multiplexing channel (at that point there's no connector yet). We could simply re-bind the parameters in the multiplexing write loop, when actually writing the parameter data. This would incur a small perf hit, and it would still not allow sharing state between the validation and writing phases; but it would allow the handler to not be thread-safe (i.e. have state).

If we do need to have state in some handler some day (imagine if NetTopologySuite's streams weren't thread-safe), we'd either have to hack some way to store state on the connector, or re-instantiate that state for every value, which could be expensive.

But given that all handlers currently look fine, I guess it's OK to just go in this direction for now, rather than compromising perf and doing something complex for some theoretical future problem. Let me know what you guys think.

/cc @NinoFloris @vonzshik @Brar

@roji roji marked this pull request as ready for review October 3, 2022 09:06
@roji roji requested a review from vonzshik as a code owner October 3, 2022 09:06
@roji roji mentioned this pull request Oct 3, 2022
@roji roji merged commit 202389a into npgsql:main Oct 3, 2022
@roji roji deleted the ConcurrentHandlers branch October 3, 2022 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement type mapping plugins at the NpgsqlDataSource level (and remove connection-level)

2 participants