Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document not blocking the thread after multiplexing execution #245

Open
vonzshik opened this issue Mar 3, 2022 · 5 comments
Open

Document not blocking the thread after multiplexing execution #245

vonzshik opened this issue Mar 3, 2022 · 5 comments
Milestone

Comments

@vonzshik
Copy link
Contributor

vonzshik commented Mar 3, 2022

[Test]
public async Task GetDataTypeName_enum()
{
	var csb = new NpgsqlConnectionStringBuilder(ConnectionString)
	{
		MaxPoolSize = 1,
		Multiplexing = true
	};
	await using var conn = await OpenConnectionAsync(csb);
	await using var _ = await GetTempTypeName(conn, out var typeName);
	await conn.ExecuteNonQueryAsync($"CREATE TYPE {typeName} AS ENUM ('one')");
	conn.ReloadTypes();
	await using var cmd = new NpgsqlCommand($"SELECT 'one'::{typeName}", conn);
	await using var reader = await cmd.ExecuteReaderAsync(Behavior);
	await reader.ReadAsync();
	Assert.That(reader.GetDataTypeName(0), Is.EqualTo($"public.{typeName}"));
}

@roji @NinoFloris

@vonzshik vonzshik added the bug label Mar 3, 2022
@vonzshik vonzshik added this to the 7.0.0 milestone Mar 3, 2022
@roji
Copy link
Member

roji commented Sep 10, 2022

Design decision: it's illegal to call sync ReloadTypes() on a multiplexing connection/data source; this should throw. Introduce a ReloadTypesAsync().

Note: if intensive computation occurs after ExecuteNonQueryAsync above, the connection won't be returned to the pool until the next yield. The only way to mitigate that would be an extra dispatch (really bad perf), or switch the dispatch between ExecutionCompletion and ReaderCompleted; but this would regress perf for small resultsets (e.g. TechEmpower) since when awaiting on ReaderCompleted the callback wouldn't yet be registered.

@roji
Copy link
Member

roji commented Sep 13, 2022

After discussing this again with @Brar and thinking about it, I'm not sure we should be doing anything here.

We know that for the multiplexing productization, we plan to make multiplexing work only through the data source command API (npgsql/npgsql#4496). At that point, open NpgsqlConnections by definition are always already bound to a connector, and so would never block on needing to get one. This means that only sync APIs on commands would need to throw when used on a multiplexing data source (as well as sync NpgsqlConnection.Open() and NpgsqlDataSource.Open(), of course).

Assuming the above is correct, we shouldn't start adding checks to throw from ReloadTypes or any other sync API, since those would no longer be necessary in 8.0. And we'd move this issue to 8.0 for making sure all command and connection open APIs have checks.

@NinoFloris @vonzshik does the above make sense or have I forgotten something?

Brar referenced this issue in Brar/npgsql Sep 13, 2022
Possibly fixes #4369 but will probably be replaced by a more general
approach.
@vonzshik
Copy link
Contributor Author

I'm OK with not doing anything as long as we document at some point this caveat with multiplexing (that is, either avoid doing any long-running cpu-bound work after queries, or yield the thread by, for example, calling await Task.Yield().

@roji
Copy link
Member

roji commented Sep 15, 2022

@vonzshik sure, though note that's orthogonal to whether we do anything in our own APIs (i.e. throw from sync ReloadTypes() when in multiplexing).

But it's true that in general, someone using multiplexing and blocking the thread right after execution would block returning the connection etc. We should note that in the multiplexing docs.

@roji
Copy link
Member

roji commented Sep 15, 2022

Moving this to the docs repo.

@roji roji transferred this issue from npgsql/npgsql Sep 15, 2022
@roji roji unassigned Brar Sep 15, 2022
@roji roji changed the title Deadlock while using multiplexing with a single connection Document not blocking the thread after multiplexing execution Sep 15, 2022
@roji roji modified the milestones: 7.0.0, 8.0.0 Sep 15, 2022
@roji roji removed the bug label Sep 15, 2022
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

No branches or pull requests

3 participants