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

Bug: Response<T>.Value is a StackOverflow #40140

Closed
wants to merge 3 commits into from
Closed

Bug: Response<T>.Value is a StackOverflow #40140

wants to merge 3 commits into from

Conversation

Porges
Copy link
Member

@Porges Porges commented Nov 20, 2023 •
edited
Loading

Noticed this while doing #40136.

Since the changes made in #35560 (this line), Response<T> has been a stack overflow as it references itself:

The active test run was aborted. Reason: Test host process crashed : Stack overflow.
Repeat 24212 times:
--------------------------------
   at Azure.Response`1[[System.Int32, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].get_Value()    
--------------------------------

This PR fixes this by making the Value property re-abstract. This breaks ApiCompat so there is a new baseline.

@Porges Porges force-pushed the response-value-broken branch from c556422 to 85e8210 Compare November 20, 2023 21:14
@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.Core

@Porges Porges changed the title Response<T>.Value is a StackOverflow Bug: Response<T>.Value is a StackOverflow Nov 20, 2023
annelo-msft
annelo-msft reviewed Nov 20, 2023
View reviewed changes
sdk/identity/Azure.Identity.Broker/api/Azure.Identity.Broker.net462.cs
@@ -9,6 +9,6 @@ public partial class SharedTokenCacheCredentialBrokerOptions : Azure.Identity.Sh
{
public SharedTokenCacheCredentialBrokerOptions() { }
public SharedTokenCacheCredentialBrokerOptions(Azure.Identity.TokenCachePersistenceOptions tokenCacheOptions) { }
public bool? IsMsaPassthroughEnabled { get { throw null; } set { } }
public bool? IsLegacyMsaPassthroughEnabled { get { throw null; } set { } }
Copy link
Member

Choose a reason for hiding this comment

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

Curious how this is related to this change?

Copy link
Member Author

@Porges Porges Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@annelo-msft no idea, but running eng\scripts\Export-API.ps1 updated this. Another PR got merged without updating generated code?

@annelo-msft
Copy link
Member

@Porges, would it be possible to share sample code that causes the stack overflow?

@Porges
Copy link
Member Author

Porges commented Nov 20, 2023 •
edited
Loading

@annelo-msft would it be possible to share sample code that causes the stack overflow?

The test that was added exhibits it (but the test code would need to be modified to hit the bug with the previous version by removing the Value override).

It’s not possible to hit this using the classes which are in the (currently) published SDKs since only subclasses of Response<T> that also define their own override of Value are used. (Which is also why making this abstract is not a breaking change to any other internal code.) It's more of a rake-to-step-on for anyone implementing a subclass of Response<T> in the future, internal or external to azure-sdk-for-net.

@annelo-msft
Copy link
Member

It’s not possible to hit this using the classes which are in the (currently) published SDKs since only subclasses of Response that also define their own override of Value are used.

I see. Thank you, that is helpful context!

Can you help us understand what your scenario is for creating a subtype of Response<T>. I am aware of some, but interested in understanding the breadth of user needs a bit better.

@Porges
Copy link
Member Author

Porges commented Nov 21, 2023

@annelo-msft Can you help us understand what your scenario is for creating a subtype of Response<T>. I am aware of some, but interested in understanding the breadth of user needs a bit better.

I don't have a current need to. This is just something I noticed in the codebase while I was implementing something else (#40136).

@annelo-msft
Copy link
Member

I don't have a current need to. This is just something I noticed in the codebase while I was implementing something else (#40136).

That makes sense. Thank you for reporting the issue!

I expect that we will be unable to take this change, since changing a member on a type from virtual to abstract is considered a breaking change, and our Azure SDK policy states that we are unable to make breaking changes to GA libraries.

If you need help finding a workaround for this issue, please let us know.

annelo-msft
annelo-msft reviewed Dec 2, 2023
View reviewed changes
sdk/core/Azure.Core/src/ResponseOfT.cs
@@ -23,7 +24,7 @@ public abstract class Response<T> : NullableResponse<T>
public override bool HasValue => true;

/// <inheritdoc />
public override T Value => Value;
public abstract override T Value { get; }
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we may also be able to address this by replacing this line with

Suggested change
public abstract override T Value { get; }
public override T Value => base.Value!;

If you would be open to making that change instead, it would not be an API-breaking change, and I think we would be happy to accept the PR. Thank you for finding and addressing this issue!

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 2, 2024

Hi @Porges. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@github-actions github-actions bot added the no-recent-activity There has been no recent activity on this issue. label Feb 2, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 9, 2024

Hi @Porges. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing /reopen if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the no-recent-activity label; otherwise, this is likely to be closed again with the next cleanup pass.

@github-actions github-actions bot closed this Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annelo-msft annelo-msft annelo-msft left review comments

@KrzysztofCwalina KrzysztofCwalina Awaiting requested review from KrzysztofCwalina

@tg-msft tg-msft Awaiting requested review from tg-msft

@JoshLove-msft JoshLove-msft Awaiting requested review from JoshLove-msft

@christothes christothes Awaiting requested review from christothes christothes is a code owner

Assignees
No one assigned
Labels
Azure.Core no-recent-activity There has been no recent activity on this issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Porges @azure-sdk @annelo-msft