Skip to content

throw an exception on id.get if session is not available #57902

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 1 commit into from

Conversation

iamkrillin
Copy link

@iamkrillin iamkrillin commented Sep 16, 2024

Throw exception on id.get

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Description

Throw an exception in DistributedSession.Id.Get if the session is not available

@ghost ghost added the area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares label Sep 16, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 16, 2024
@amcasey
Copy link
Member

amcasey commented Sep 17, 2024

It looks like the unavailability of a session is already signalled by an empty Id. Is that not sufficient?

@iamkrillin
Copy link
Author

The problem I was running into was two fold:

  1. The documentation says the Id returned is unique, which strictly speaking, if there is a possibility for it to return "" is not always true, but the bigger problem was..
  2. When using sqlserverdistributedcache, if there is a problem with the setup (server not available, table missing, etc). The error gets eaten then logged to the console (which is not ideal when using IIS hosting)

These two things combined lead to a scenario where things looked to be working, but were not working. When I was working on this PR I though it was better to throw an exception since that would make it very clear that things were not working, but returning null would also work

Either of these would be preferable to eating a error, then returning a value IMO

@amcasey
Copy link
Member

amcasey commented Sep 17, 2024

I'm not actually familiar with this type, but the current behavior certainly appears to be intentional. If we were going to move to a model where it throws, wouldn't it make more sense to just add a throw in Loads catch block?

Similarly, it would have been easy to give each failed session its own guid, so I assume they were grouped together with the id "" on purpose. (Whether that purpose is valid, I have no idea, though it's a common enough pattern.)

@iamkrillin
Copy link
Author

That's a fair enough point, I did consider adding a throw into the catch in Load, I ultimately decided against it since Load is called in many places and I wanted my change to be as small as possible.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Sep 24, 2024
@amcasey
Copy link
Member

amcasey commented Oct 4, 2024

I was hoping an expert would chime in, but I'm not sure there is one for this type. 😆

Barring that, I think I'd prefer to just set it to null, rather than string.Empty in Load, which should cause it to use a fresh guid. Having said that, I can also see the value in just leaving it. If there is a session ID and it just couldn't be loaded, assigning a fresh one is a little strange. It looks like the expectation of the API is that the other properties will be disregarded if IsAvailable is false.

If you need more explicit error behavior in your app, do you have the option of either checking IsAvailable or switching to LoadAsync, which appears to throw when you want it to?

@iamkrillin
Copy link
Author

I changed my application to check IsAvailable before opening this pull request, but figured if this behavior impacted me, perhaps it is impacting others. My Application was checking for null before as an error condition so that makes me think at some point in the past perhaps this API returned null in that scenario. My application has been around since .net 3 and is being updated to .net core so it may have been long ago.

@amcasey
Copy link
Member

amcasey commented Oct 7, 2024

if this behavior impacted me, perhaps it is impacting others

Thanks for taking time to contribute! 😄

it may have been long ago

It certainly seems possible that the sentinel used to be null and changed to string.Empty sometime in the last decade. If we were designing the API now, I think there would be a strong argument in favor of making it throw. In fact, I'm guessing that did actually happen with LoadAsync was added.

In my personal opinion, the best way forward would be to update the doc comments to explain that IsAvailable should be called before Id, etc. I would also consider having Load set it to null to cause a new ID to be generated, though I think the value of that is diminished pretty substantially if the check-IsAvailable-first convention is made explicit.

Copy link
Contributor

@mkArtakMSFT mkArtakMSFT left a comment

Choose a reason for hiding this comment

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

Thanks for your patience.
It looks like the conversation with @amcasey landed on potentially documenting the current behavior, instead of adding this check. However, I can see the value of being explicit and given where we are in the release I wonder if it's safe to take this change. So, no objection from me, but someone who is more familiar with the codebase should share their opinion too.

if(IsAvailable == false)
{
throw new InvalidOperationException("Session not currently available");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The IsAvailable getter already calls Load() internally.
If the plan is to leave this check in place, let's remove the Load() from the line above.

@mkArtakMSFT
Copy link
Contributor

@JamesNK are you by any chance familiar with this code?

@mkArtakMSFT
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@JamesNK
Copy link
Member

JamesNK commented Dec 19, 2024

@JamesNK are you by any chance familiar with this code?

No, never used it.

IMO, I don't think we should change the behavior of the property. We could break people who depend on the current behavior. If we did change the current behavior, then I think an option would be required to revert it if needed. It starts to become a lot of work for not a huge benefit. And, I'm not sure we fully understand why this choice was made in the first place, which makes me hesitant to change it.

Instead of this change, I think we should update the documentation to say an Id is empty if there isn't a session. Either LoadAsync should be explicitly called or IsAvailable should be checked. Basically, I agree with what @amcasey said here. It's not a great API, but it's what we have to live with.

@mkArtakMSFT
Copy link
Contributor

Thanks @JamesNK.
Appreciate your input.

Instead of this change, I think we should update the documentation to say an Id is empty if there isn't a session. Either LoadAsync should be explicitly called or IsAvailable should be checked

@iamkrillin would you like to update your PR to do this instead?

@mkArtakMSFT mkArtakMSFT added the pr: pending author input For automation. Specifically separate from Needs: Author Feedback label Dec 19, 2024
@mkArtakMSFT
Copy link
Contributor

Actually, I'm going to close this for now. @iamkrillin feel free to reopen if you'd like to repurpose this for the documentation change.

@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview1 milestone Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun pr: pending author input For automation. Specifically separate from Needs: Author Feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants