Repository navigation
Setting NpgsqlReadBuffer.Timeout property to Timeout.InfiniteTimeSpan gets overridden in setter #6122
Description
I'm not 100% sure if this is intentional or not, but if you were to set the Timeout property of an NpgsqlReadBuffer instance to Timeout.InfiniteTimeSpan, the result is value will be set to TimeSpan.Zero since the value of the InfiniteTimeSpan struct is -1ms.
Link to source, relevant block:
internal TimeSpan Timeout
{
get => _preTranslatedTimeout;
set
{
if (_preTranslatedTimeout != value)
{
_preTranslatedTimeout = value;
if (value == TimeSpan.Zero)
value = InfiniteTimeSpan;
else if (value < TimeSpan.Zero)
value = TimeSpan.Zero;
Debug.Assert(_underlyingSocket != null);
_underlyingSocket.ReceiveTimeout = (int)value.TotalMilliseconds;
Cts.Timeout = value;
}
}
}This can be worked around by just setting Timeout to TimeSpan.Zero, but this isn't at all obvious. You could check for value != Timeout.InfiniteTimeSpan, but that might have unintended consequences if someone were specifying -1ms for the timeout value expecting it to result in TimeSpan.Zero? Maybe there just needs to be some XMLDoc comments added to NpgsqlBinaryImporter.Timeout pointing this out?
Maybe relevant but probably not backstory on how I wound up here:
I ran into this because I was working on bulk inserts using this lib and would get timeouts if I inserted enough data. After running with local versions of both npgsql and the EFCore.BulkExtensions.MIT library, I figured out that the timeout was coming from NpgsqlBinaryImporter.Complete(). I played around with manually setting NpgsqlBinaryImporter.Timeout to Timeout.InfiniteTimeSpan immediately before calling Complete() but saw no effect and dug more and saw that I really needed to set it to TimeSpan.Zero to have the effect I wanted.
Activity
vonzshik commentedon May 30, 2025
It's kinda intentional but not really.
At the time when I was working on this, the only way to actually change
NpgsqlReadBuffer.Timeoutwas to execute a query withNpgsqlCommand.CommandTimeout, since COPY operations were lacking any sort of API for that.And here's the catch,
CommandTimeoutcan't be negative: you can either set it to 0 (infinity) or some positive value, unlike sockets, where both 0 and -1 are allowed for infinity. Then we also haveCancellationTimeout, where -1 is to exit immediately and 0 is for infinite wait.Since we also have to use
CancellationTokenSourcefor async path (where -1 is infinity and 0 is to exit immediately), at the time I thought it was better to do translation inNpgsqlReadBufferrather than try to track each path accessing the property (and it's not like there was another API other thanCommandTimeoutandCancellationTimeout).Thinking a bit more, maybe it's better to revisit this and only do translation at specific points, rather than globally at
NpgsqlReadBuffer.Timeout, though it will be a breaking change (but since no one raised this before, maybe it's fine?). @roji what do you say?roji commentedon Jun 1, 2025
Yeah, from the user's perspective I agree that setting NpgsqlBinaryImporter.Timeout to Timeout.InfiniteTimeSpan should work as intended (i.e. no timeout/infinite timeout rather than immediate timeout/fail). While technically a breaking change I agree we should be fine to do this - I actually see it as more of a bugfix...
In fact, I'm wondering whether it makes sense to make infinite timeout the default for COPY operations, which are by their nature supposed to be long (not sure)... Infinite timeouts by default aren't a good idea since if there's a network partitioning the user is left stuck and wondering, but if people tend to explicitly set it (like @cokert above) it's just a nuisance. Any thoughts?
cokert commentedon Jun 1, 2025
I have no idea the full path between my app code and this block, but the real surprise for me was the only way I could find to set this from my app was from the connection string's
Command Timeoutproperty. TheDbContext'sCommandTimeoutvalue as set by.UseNpgsql(..., options => options.CommandTimeout(...)wasn't being picked up for this timeout value.But really, since the timeout that was affecting me is when
NpgsqlBinaryImporter.Complete()gets called, I was wondering if there's any good reason this shouldn't always be infinite? There's no real upper bound on the amount being imported, so there's no way to know how long postgres will take to respond/what a sane timeout value should be. Code doing this should be on some form of background processing -- ie, no one's should be calling this in a context where a user's waiting on a response. So really, I can't think of a good reason for this to have a timeout at all, but maybe I'm missing something?roji commentedon Jun 1, 2025
That's most likely because EF sets that on NpgsqlCommand.CommandTimeout, but there's no command when doing COPY. The
Command Timeoutconnection string parameter is a bit badly-named in that sense, since it affects both commands and COPy.@cokert see my comment just above - infinite timeouts can be a problem if there's a network issue, and then your application really is stuck forever. I do think there's a case for making COPY timeouts infinite (much more than regular command timeouts), but I'm not sure.
cokert commentedon Jun 1, 2025
vonzshik commentedon Jun 1, 2025
Yes.
npgsql/src/Npgsql/Internal/NpgsqlConnector.cs
Lines 2539 to 2542 in 892774f
COPY operations are almost never a part of app's usual lifecycle (they're called either on migration or a part of initialization), so I think they should have infinite timeout by default. I wonder whether it makes sense to also introduce another timeout parameter (
CopyTimeout?) to connection string, since the use case for usual queries and COPY operations is quite different.roji commentedon Jun 1, 2025
@vonzshik yeah, that all makes sense to me. Any thoughts @NinoFloris?
Remove timeout translation from NpgsqlReadBuffer (npgsql#6126)