-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
RegexMatchTimeoutException for SqlServerMigrationsSqlGenerator #16175
Comments
@bricelam can you please explain the workaround? |
Triage: We believe the root cause of the issue is what @oatsoda mentioned above: For a sufficiently long input and a sufficiently slow CPU, the regular expression processing times out before it can complete the work. We have aggressively added timeout to any regex usage across our codebase as a blanket measure to mitigate catastrophic backtracking. When doing so actually causes functional issues we need to consider other alternatives. For example:
Adding a timeout parameter to the Sql() method doesn't seem to make sense (users shouldn't need to know that there is a regex doing work). All that said, we don't have much time left in 3.0 and we wish to punt this issue to the backlog. In the meantime we believe that customers that hit this can implement themselves the split in application code and make multiple calls to the Sql() methods rather than pass long SQL strings with multiple GO to a single call. |
@divega I will give it a go splitting it. Thanks. |
Here's our splitting code copied into an extension method. You can tweak it as needed: static void LargeSql(this MigrationBuilder migrationBuilder, string sql, bool suppressTransaction = false)
{
var batches = Regex.Split(
Regex.Replace(
sql,
@"\\\r?\n",
string.Empty,
default,
TimeSpan.FromMilliseconds(1000.0)), // TODO: Increase timeout as needed
@"^\s*(GO[ \t]+[0-9]+|GO)(?:\s+|$)",
RegexOptions.IgnoreCase | RegexOptions.Multiline,
TimeSpan.FromMilliseconds(1000.0)); // TODO: Increase timeout as needed
for (var i = 0; i < batches.Length; i++)
{
if (batches[i].StartsWith("GO", StringComparison.OrdinalIgnoreCase)
|| string.IsNullOrWhiteSpace(batches[i]))
{
continue;
}
var count = 1;
if (i != batches.Length - 1
&& batches[i + 1].StartsWith("GO", StringComparison.OrdinalIgnoreCase))
{
var match = Regex.Match(
batches[i + 1], "([0-9]+)",
default,
TimeSpan.FromMilliseconds(1000.0)); // TODO: Increase timeout as needed
if (match.Success)
{
count = int.Parse(match.Value);
}
}
for (var j = 0; j < count; j++)
{
var batchSql = batches[i];
if (i == batches.Length - 1)
{
batchSql += Environment.NewLine;
}
migrationBuilder.Sql(batchSql, suppressTransaction);
}
}
} |
Thanks @bricelam I will give it a go. @divega I can understand that another parameter might be a bit ugly as consumers won't really want to know that a |
@bricelam I've used your workaround to increase the timeout but I think there could be somewhere else that a Regex is applied. It gets past the
|
I was afraid of that. If there aren't actually any GO statements, pre-splitting won't help. Need to override this code instead. class MySqlServerMigrationsSqlGenerator : SqlServerMigrationsSqlGenerator
{
protected override void Generate(
SqlOperation operation,
IModel model,
MigrationCommandListBuilder builder)
{
// TODO: Reimplement without timeouts
}
} optionsBuilder
.UseSqlServer(...)
.ReplaceService<IMigrationsSqlGenerator, MySqlServerMigrationsSqlGenerator>(); |
@bricelam Yeah, I do have GO statements, but I had to reduce it down to each batch being no more than 25 lines (I have about 100,000 in total) to avoid the timeout. Significantly reduced resources running on an Azure Function unfortunately. Thanks though, the override of the |
Version: 2.2.4
Provider: SqlServer
I have some fairly large initial Migrations to set up some static data (approx 100,000 lines):
The error seems to be coming from the fact that there is a limited 1000ms timeout on the regex parsing of this.
https://github.com/aspnet/EntityFrameworkCore/blob/f9d1df40fed1e1d7780553c3f9c1813e81eca1aa/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs#L1105
To mitigate this I added more GO statements into my SQL to avoid it, which worked fine. That was until I deployed it into an Azure Function and now presumably because of the lower CPU/Mem I am running into issues.
Obviously I am looking in to way I could reduce the 100,000 lines and/or increasing resources, but it might also be helpful if this Timeout was overidable somehow (if the value wasn't set at all I could have used the
REGEX_DEFAULT_MATCH_TIMEOUT
Environment variable), perhaps in themigrationBuilder.Sql(
method call?The text was updated successfully, but these errors were encountered: