[bug] Pop doesn't remove value if w.Write happened before sm.Pop

[jackielii]

This is caused by https://github.com/alexedwards/scs/blob/7e11d57e8885fd38c604d4f79f0eef355650b997/session.go#L158-L160

As demostrated in #216 https://github.com/alexedwards/scs/blob/8650757c94bcb44a97f0f206d0ca880cb126867c/session_test.go#L351-L391

If Pop happens after a w.Write, sw.written would be true. Therefore the commit would never happen

The second /get should have empty response, but it got bar instead:

go test github.com/alexedwards/scs/v2 -run '^TestFlushPop$' -timeout 30s -v -count 1
=== RUN   TestFlushPop
=== PAUSE TestFlushPop
=== CONT  TestFlushPop
    session_test.go:379: want "yhKe4NJJYf3SgN2bcxyKgbBApVF312oIEg2Bo-qpC2U"; got ""
    session_test.go:384: want ""; got "bar"
--- FAIL: TestFlushPop (0.00s)
FAIL
FAIL	github.com/alexedwards/scs/v2	0.011s
FAIL

[jum]

In general, all HTTP headers must be set and finalized before any content is written. This means you basically have to buffer the output if you intent to modify the session while producing output (very common when popping flash messages). This has bitten me quite a few times.

[jackielii]

In general, all HTTP headers must be set and finalized before any content is written. This means you basically have to buffer the output if you intent to modify the session while producing output (very common when popping flash messages). This has bitten me quite a few times.

Yes, I spend one afternoon tracking down the problem. My call stack was too deep, I didn't even suspect session manager at first.

I think even if it's decided that this is expected, I prefer to have a error or even panic for this so that I know what not to do.

In addition, my initial issue was the message not removed after Pop call. So I'm fine with the set-cookie header not present, but the values have to be removed from store.

[alexedwards]

I've been thinking about this more just now, and it's a bit of a tricky problem.

At the moment, SCS commits the session changes to the DB and sets the cookie header when the first response write is taking place.

And it only does this at the first write. As the issue rightly points out, subsequent modifications to the session data will not automatically be committed to the DB at any point after that first write. At the very least, the package documentation should highlight this behavior better.

If we change the package to also automatically commit session data to the DB after that first write (like in the PR https://github.com/alexedwards/scs/pull/216), we have a problem with error handling. The Commit method may return an error (for example, the DB is temporarily unavailable) and that has to be dealt with properly. The options I can see are:

1. Caliing ErrorFunc here to log the error and send a HTTP response. This doesn't work because the HTTP headers and potentially a partial response have already been sent.
2. We could panic(err) instead, but that feels to me like an inappropriate use of panic because things like failing to write to a DB are a normal and expected operational error, not an unexpected logic or programmer error.
3. We could make SessionManager accept a NoResponseErrorFunc function that specifies the action to take for errors that happen after the HTTP headers for the response have been sent, and call that instead of ErrorFunc. I have mixed feelings about this idea, I think it would work but I don't like the complexity it adds.

There are some other problems too. The suggested change in PR #216, or something similar to it, would mean that two DB calls via Commit would be made in the same request cycle, even for simple handlers that just make one write, which doesn't seem ideal. I think it also introduces a subtle bug when idle timeouts are being used, where the expiry time set in the DB by the 2nd commit would be out of sync with the expiry time set in the session cookie.

Considering this all, I'm inclined to do the following things:

1. Leave the package behavior as-is, so that session data changes are only automatically commited to the DB when the first response write is taking place.
2. Document this behavior more clearly.
3. Document that if you are making multiple writes, and modifying the session data after the first write, it is possible to manually commit the session data to the DB by calling Commit.
4. Add a Committed status to the session data which is set immediately after successful commit. Any subsequent modifications to the session data will update this status to Modified. Add a check right at the end of the LoadAndSave function to make sure that the status is not Modified, and if it is then panic with a message like "the session data contains uncommited modifications, probably because you are modifying the data after the response headers have already been written; if you need to modify the session data after the response headers are written, please call the Commit method after making the modification to persist the changes". I think it's acceptable to panic here, because it's an unexpected programmer/logic error, rather than an expected operational one.
5. Fix the expiry time sync issue by moving the expiry time calculation into Load, not Commit. I've opened https://github.com/alexedwards/scs/issues/251 for this change.

[alexedwards]

The expiry time issue described in point 5 has been fixed in 481af0a28857c8d0fbb2709d9623b03e7f67848b. The uncommitted modifications check and panic described in point 4 has been implemented in 2060319a03b0788d7b4562a865c57b42061c10bc. The documentation improvements described in points 2 and 3 have been made in 209de6e426de9259665975ce16b91331d228f052.

I'm going to close this issue as completed, but if you disagree then please let me know.