panic in getSessionDataFromContext makes this package hard to work with

[deltamualpha]

Hello,

I was a happy user of the v1 of this package, and recently decided to upgrade to v2, however, I found some of the new behaviors extremely difficult to work with.

One piece in particular was the fact that the package throws a panic if you try and load a session from a request that doesn't have a session attached: getSessionDataFromContext. Previously, I could attempt to load a session blindly, and based on the response from the function, I'd know if the request had a session attached. Now, I have to make absolutely sure that I've got the LoadAndSave middleware inserted all over the place.

Panics thrown from userland are pretty unidiomatic in go, I feel, and the only way to really handle them is by doing the defer-recover dance, which is messy and error-prone. I'm likely going to migrate to another session-management library.

[alexedwards]

Hi : )

The decision to panic in getSessionDataFromContext(), rather than return an error, in v2 was something I thought about carefully and wasn't a choice I made lightly.

The way I view it, you should only be calling sessionManager.XXXX(ctx) when you logically expect the ctx you are passing in to actually contain a session. It's a bit like calling a function like io.WriteString() without actually passing a valid io.Writer as the first parameter --- if you call io.WriteString(nil, "foo") you'll get a runtime panic.

Panics for expected errors (e.g. database query timeout, a network resource being unavailable, or bad user input) that may happen at runtime are unidiomatic in Go. In contrast, panics for unexpected errors (e.g. a logical error --- like trying to access an out-of-bounds index in a slice, or trying to close an already-closed channel --- or trying to use language or package features in an unintended way) are much more acceptable. There's a lot of precedent for this in the std lib packages.

Calling sessionManager.XXXX(ctx) when no sesson data exists in the provided context firmly feels to me like a unexpected logic error in a codebase, and I don't think that panicing in that scenario is unidiomatic.

Personally, I usually wrap all my main application routes (i.e. all those that aren't serving static files, or performing some other special function) with the LoadAndSave() middleware and it works fine. Using something like alice (https://github.com/justinas/alice) or a router which supports middleware 'groups' like chi can be helpful here and saves you from having to remember to wrap individual handlers.

[theckman]

@alexedwards how can I inspect a context to know whether or not it has the appropriate session inside, to selectively avoid invoking your code (thus seeing the panic)?

[alexedwards]

@theckman Can you explain the scenario in which would you need to do that? Why would you want to perform an session operation on a context which may or may not contain a session?

[deltamualpha]

To put some more color on my use-case: I'm writing an authentication microservice that needs to collate nearly a half-dozen different ways that a request could be authenticated. I have handlers and middleware in that service that want to treat the presence or absence of a "valid" session -- not just an empty, uninitialized one -- as a signal, and fall through various authentication methods without having to do a weird recover() dance. (In other words, if a user provided a session cookie is a meaningful distinction for me, not just "is there a value in the session or not".)

I'm not sure I buy the difference between "expected" and "unexpected" errors. Panics should be, in my opinion, reserved for cases where there's truly no reasonable way for execution to continue. Everything else is just an error that can be returned as a value and handled as part of the normal execution flow. Throwing a panic because an expected value is not present in a context, which is what this case boils down to, feels especially unpleasant.

[alexedwards]

@deltamualpha

I'm not sure I buy the difference between "expected" and "unexpected" errors. Panics should be, in my opinion, reserved for cases where there's truly no reasonable way for execution to continue.

Respectfully, I think that is the case here. I'll try to explain my thinking on this further.

Let's say that we don't panic, and return a ErrNoSession error from getSessionDataFromContext() instead. As things stand, this is all we can do here. All we know is that there is no session in the context -- we can't return a more meaningful error which identifies why there is no session in the context.

Now, the context may not contain a session for a number of reasons. A few possibilities are:

a) The programmer deliberately didn't load a context into the session, and actively doesn't want one to be there.
b) The programmer forgot to wrap the route with the LoadAndSave() middleware, which means the context isn't being loaded into the session when it should be.
c) There is a bug somewhere in the scs package which means the context isn't being loaded into the session when it should be.

There might be more reasons too.

If I'm following your example correctly, then in your specific scenario, if getSessionDataFromContext() returned a ErrNoSession value, which you then used in your logic flow as a signal this is potentially a problem. You might take it as a signal that no session token was provided (reason a), but the reality might be that there is a bug in SCS (reason c) or elsewhere in your codebase (reason b) which means that you fall through to another authentication method, when actually you should be ceasing execution of your program and returning a 500 error because there is a bug which needs fixing. Allowing execution to continue down could lead to some unexpected behaviour in your system.

To be fair, in this scenario the same problem exists if you use the panic as the signal too.

Even outside of your scenario, in a more 'standard' use case of SCS, we can't tell the difference between reasons b and c.

One of the design decisions of SCS is that getSessionDataFromContext() expects there to be a valid session in the context. If there isn't, then my view is that this is an unexpected error, the program is operating in an unknown state, we don't know the underlying reason behind the problem, and that allowing the program to continue execution is dangerous. On that basis, I think that panicing is an appropriate thing to do.

[alexedwards]

One option could be to add a new SessionManager.ValidContext(ctx) method which returns true if the provided context contains a valid session. You could then use this to check for the presence of a valid session before calling any other methods (thereby avoiding the panic in your workflow). The documentation for this method could include the necessary warnings and explain the risks of using it in your code.

[alexedwards]

If there's any desire to see SessionManager.ValidContext(ctx) added, please comment and I'll reopen this issue.

[fouadyahyaoui]

@alexedwards What about allowing the user to configure if getSessionDataFromContext() should panic or not?
For instance adding a mustGetSessionDataFromContext() function as default, which panics, and a getSessionDataFromContext() function which does not panic, but does return a ErrNoSession error.