alexedwards / scs

HTTP Session Management for Go
MIT License
2.12k stars 165 forks source link

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

Open jackielii opened 3 months ago

jackielii commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.