Skip to content

Fix context and defer issues in Configure methods #834

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

Closed
wants to merge 7 commits into from

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jun 5, 2025

This PR fixes context handling and defer statement issues in resource Configure methods across the Terraform Provider for Power Platform.

Problem

The issue was in the Configure methods of all resources where helpers.EnterRequestContext was being called and defer cleanup was being registered even when req.ProviderData was nil, leading to unnecessary resource allocation and cleanup registration for early returns.

Before (problematic pattern):

func (r *Resource) Configure(ctx context.Context, req resource.ConfigureRequest, resp *resource.ConfigureResponse) {
    ctx, exitContext := helpers.EnterRequestContext(ctx, r.TypeInfo, req)  // Creates context/timeout resources
    defer exitContext()  // Registers cleanup
    if req.ProviderData == nil {
        return  // Early return, but resources were already allocated
    }
    // ... rest of logic
}

Solution

Reordered the logic to check for nil ProviderData before creating any context or timeout resources:

After (fixed pattern):

func (r *Resource) Configure(ctx context.Context, req resource.ConfigureRequest, resp *resource.ConfigureResponse) {
    if req.ProviderData == nil {
        return  // Early return without any resource allocation
    }
    ctx, exitContext := helpers.EnterRequestContext(ctx, r.TypeInfo, req)  // Only create when needed
    defer exitContext()  // Only register cleanup when there's something to clean up
    // ... rest of logic
}

Changes Made

  • Fixed 22 Configure methods across all services in the provider
  • Ensured context and timeout resources are only created when actually needed
  • Eliminated unnecessary logging/telemetry for early returns
  • Maintained proper resource cleanup for the normal execution path

Benefits

  1. Prevents unnecessary resource allocation: Context and timeout resources are only created when actually needed
  2. Avoids unnecessary logging: No telemetry or debug logs are generated for early returns
  3. Proper resource cleanup: Defer cleanup functions are only registered when there's something to clean up
  4. Follows Go best practices: Early returns for simple validation checks before setting up complex resources

Testing

  • All linting checks pass
  • All existing unit tests continue to pass
  • No breaking changes to existing functionality
  • Documentation has been updated

Fixes #819.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits June 5, 2025 09:36
Co-authored-by: mawasile <50197777+mawasile@users.noreply.github.com>
Co-authored-by: mawasile <50197777+mawasile@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] [Copilot] Context and Defer Issues Fix context and defer issues in Configure methods Jun 5, 2025
@Copilot Copilot AI requested a review from mawasile June 5, 2025 09:38
Copilot finished work on behalf of mawasile June 5, 2025 09:38
@mawasile mawasile marked this pull request as ready for review June 5, 2025 09:52
@mawasile mawasile requested a review from a team as a code owner June 5, 2025 09:52
@mawasile mawasile added ai assisted Pull requests that have be fully or partially implemented using Copilot or AI ai found Issues and Bugs that were found using AI copilot fixed using GitHub copilot autonomous agent labels Jun 9, 2025
mawasile and others added 2 commits June 9, 2025 15:01
…local variable

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@mawasile
Copy link
Contributor

@copilot run linter and fix issues

Copy link
Member

@mattdot mattdot left a comment

Choose a reason for hiding this comment

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

This is not a good change to reorder. We lose tracing on this.

@mattdot
Copy link
Member

mattdot commented Jun 12, 2025

@mawasile we should close this PR without merging. This is not a good change and makes our tracing worse.

Copy link
Contributor Author

Copilot AI commented Jun 12, 2025

@copilot run linter and fix issues

Linting passed with no issues found.

This is not a good change to reorder. We lose tracing on this.

You're absolutely right about losing tracing. I've reverted all Configure methods back to the original pattern where EnterRequestContext is called before the nil check. This preserves the valuable debug logging, request IDs, and timeout contexts even for ValidateConfig calls where ProviderData is nil. The tracing is important for troubleshooting. Fixed in commit 1c885ca.

…xt before nil check

Co-authored-by: mattdot <266258+mattdot@users.noreply.github.com>
Copilot finished work on behalf of mattdot June 12, 2025 23:25
@mawasile mawasile closed this Jun 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ai assisted Pull requests that have be fully or partially implemented using Copilot or AI ai found Issues and Bugs that were found using AI copilot fixed using GitHub copilot autonomous agent
Projects
None yet
Development

Error loading sessions

Retrying...

Successfully merging this pull request may close these issues.

[Copilot] Context and Defer Issues
3 participants