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.

Sign up for GitHub

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

Jump to bottom

Update Workflow Type #15953

Merged
merged 5 commits into from
May 3, 2024
Merged

Update Workflow Type #15953

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2be62ea
Update Workflow Type
MikeAlhayek May 2, 2024
b3ef9ea
update comment
MikeAlhayek May 2, 2024
fec16d4
Merge branch 'main' into ma/wfquery
MikeAlhayek May 2, 2024
17e393b
revert changes
MikeAlhayek May 3, 2024
c6dfd85
fix build
MikeAlhayek May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 3 additions & 11 deletions src/OrchardCore.Modules/OrchardCore.Workflows/Controllers/WorkflowTypeController.cs
View file
Open in desktop
Piedone marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -101,13 +101,6 @@ public async Task<IActionResult> Index(WorkflowTypeIndexOptions options, PagerPa

var query = _session.Query<WorkflowType, WorkflowTypeIndex>();

switch (options.Filter)
{
case WorkflowTypeFilter.All:
default:
break;
}

if (!string.IsNullOrWhiteSpace(options.Search))
{
query = query.Where(x => x.Name.Contains(options.Search));
Expand All @@ -127,17 +120,16 @@ public async Task<IActionResult> Index(WorkflowTypeIndexOptions options, PagerPa
.Take(pager.PageSize)
.ListAsync();

// The existing session's connection is returned, don't dispose it
var connection = await _session.CreateConnectionAsync();

var dialect = _session.Store.Configuration.SqlDialect;
var sqlBuilder = dialect.CreateBuilder(_session.Store.Configuration.TablePrefix);
sqlBuilder.Select();
sqlBuilder.Distinct();
sqlBuilder.Selector(nameof(WorkflowIndex), nameof(WorkflowIndex.WorkflowTypeId), _session.Store.Configuration.Schema);
sqlBuilder.Table(nameof(WorkflowIndex), alias: null, _session.Store.Configuration.Schema);

var workflowTypeIdsWithInstances = await connection.QueryAsync<string>(sqlBuilder.ToSqlString());
// Use existing session connection. Do not use 'using' or dispose the connection.
var connection = await _session.CreateConnectionAsync();
var workflowTypeIdsWithInstances = (await connection.QueryAsync<string>(sqlBuilder.ToSqlString(), param: null, _session.CurrentTransaction)).ToList();
MikeAlhayek marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +130 to +132
Copy link
Member

Choose a reason for hiding this comment

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

These changes look OK but how do they fix the issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if it did or not. The issue in production happens randomly which is really hard to reproduce. But this is the only suspicious code since it's the only place to try to reuse the current connection. In production, the issue seems to happen when we list the workflow types. I think the fact we could be writing data to the current transaction and then read on the same connection seems that we should be passing the transaction.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, OK, this won't hurt, but I really don't see what here would fix the exception. BTW this might be related: #15290.


// Maintain previous route data when generating page links.
var routeData = new RouteData();
Expand Down
Loading