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

Increased data provider timeout during install #2645

Merged
merged 5 commits into from
Apr 16, 2019
Merged

Increased data provider timeout during install #2645

merged 5 commits into from
Apr 16, 2019

Conversation

daguiler
Copy link
Contributor

Summary

The default 30 second timeout of the SQL data provider is often not enough during install and generates timeout errors.
This is now happening to me when using the /Install/Install.aspx page to refresh test environments. I can't say this has happened to me using the wizard though.

Screenshots

install-timeout-1

Here's what I see in the DotNetNuke.Data.log.resources file:

System.Data.SqlClient.SqlException (0x80131904): Execution Timeout Expired.  The timeout period elapsed prior to completion of the operation or the server is not responding.

...

Add 6 rows to dbo.[ContentTypes]
Add 146 rows to dbo.[EventLogTypes]
Add 3 rows to dbo.[FolderMappings]
Add 1 row to dbo.[IPFilter]
Add 35 rows to dbo.[Journal_Types]
Add 1 row to dbo.[Languages]
Add 5126 rows to dbo.[Lists] ---> System.ComponentModel.Win32Exception (0x80004005): The wait operation timed out
   at System.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at System.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
   at System.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
   at System.Data.SqlClient.SqlCommand.RunExecuteNonQueryTds(String methodName, Boolean async, Int32 timeout, Boolean asyncWrite)
   at System.Data.SqlClient.SqlCommand.InternalExecuteNonQuery(TaskCompletionSource`1 completion, String methodName, Boolean sendToPipe, Int32 timeout, Boolean& usedCache, Boolean asyncWrite, Boolean inRetry)
   at System.Data.SqlClient.SqlCommand.ExecuteNonQuery()
   at DotNetNuke.Data.SqlDatabaseConnectionProvider.ExecuteNonQuery(String connectionString, CommandType commandType, Int32 commandTimeout, String query)
   at DotNetNuke.Data.SqlDataProvider.ExecuteScriptInternal(String connectionString, String script, Int32 timeoutSec)
ClientConnectionId:6f58e27b-d594-49fd-9312-1a3f385f0a09
Error Number:-2,State:0,Class:11

I tried to find a way to change the default command timeout in the connection string, but it's not possible. So this solution uses an existing overload of ExecuteScript which accepts a timeout.
Setting a 5 minute timeout fixes the issue.

mitchelsellers
mitchelsellers previously approved these changes Mar 26, 2019
Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (Would prefer to delay this to next release)

Copy link
Contributor

@ohine ohine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this when setting up the unit tests to run in DevOps. The main issue isn't it's timing out. The actual issue is the size and structure of the data file. It's 6750 lines of SQL that's expected to run at once in a single command (30s). If we split this up and add a few GO keywords. Dnn will split the file by GO's and run it in smaller sections which should improve performance quite a bit.

I'd much rather us fix the issue instead of just allowing the process to run up to 5 minutes.

@ohine
Copy link
Contributor

ohine commented Mar 26, 2019

This is also why we cannot install dnn on a small AppService instance in Azure.

@ohine ohine changed the base branch from release/9.3.1 to release/9.3.2 April 3, 2019 18:16
@daguiler
Copy link
Contributor Author

I implemented your suggestions @ohine. Can you check?
I tested the new install and it works fine, not getting any timeouts.

@daguiler
Copy link
Contributor Author

I had to copy/paste the variable declarations in each batch for 2 reasons:

  1. variables don't survive beyond GO statements
  2. I thought of storing vars in a temp table, as suggested in SO, but it seems our data provider uses separate connections for each batch, so temp tables don't survive either.

Copy link
Contributor

@sleupold sleupold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO there is no benefit combining indexes and system records in a single script.
we would gain flexibility, if we introduce a DotNetNuke.index.sqldataprovider file to prevent the timeout.

GO

-- minimum date to set for all creted on last modified on columns
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am asking myself, what this comment means…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to set these value only once on top of the script - there is a high risk of overlooking one of the occurrences. Would it be possible to restructure the script to have these values first before the first GO? Otherwise we should improve our system of DNN placeholders for SQL and provide it as additional variables for substitution e.g. {DefaultCreatedBy} etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that's what I explained in my previous comment. These vars were originally defined at the beginning of the script, but that worked because there were no GO statements. I tried temp tables also, but didn't work either because temp tables don't survive the connection.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am asking myself, what this comment means…

Agreed. Removed

@daguiler daguiler added esw Issues reported by ESW team or Evoq customers and removed esw Issues reported by ESW team or Evoq customers labels Apr 13, 2019
@valadas
Copy link
Contributor

valadas commented Apr 14, 2019

@dnnsoftware/approvers Can we assign this to the 9.3.2 milestone ? I also had a few customers with this issue, it does not happen on all servers though, but would be nice to include that in 9.3.2 if possible.

@daguiler daguiler added this to the 9.3.2 milestone Apr 14, 2019
@ohine
Copy link
Contributor

ohine commented Apr 16, 2019

This looks great, thanks for making the changes @daguiler

Copy link
Contributor

@donker donker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@ohine ohine merged commit c9c4b75 into dnnsoftware:release/9.3.2 Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esw Issues reported by ESW team or Evoq customers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants