Skip to content
This repository has been archived by the owner on Jul 29, 2024. It is now read-only.

KernelEvents::TERMINATE will dispatch twice after push HttpCache to stack #10

Closed
thbourlove opened this issue Nov 19, 2014 · 3 comments
Closed

Comments

@thbourlove
Copy link

No description provided.

@igorw
Copy link
Contributor

igorw commented Nov 19, 2014

First of all, thanks so much for reporting this!

This highlights a pretty interesting flaw in the current design of the stack. The stack assumes that every middleware will handle its own layer of termination. The stack just triggers every terminate() individually. However, this is not the case. Since a middleware is a decorator, if it implements terminate(), it will of course also call terminate() on the next layer!

As a result, it will be called once by stack, and once by the cache middleware itself.

The reason for doing this in the first place, is that terminable is optional. So because not every middleware of the chain has to implement it, the chain can be broken. The point of the stack is to never break this chain, and call all of the terminable middlewares.

I have some ideas for fixing the problem, but it is going to get a bit complicated.

@igorw
Copy link
Contributor

igorw commented Nov 19, 2014

It's... complicated.

igorw added a commit that referenced this issue Nov 19, 2014
* Should not call terminate() more than once (it was potentially factorial of stack size)
* Allow middlewares to delegate terminate() calls
* Increases stack size by number of non-terminable middlewares wrapping a terminable

If possible, we should find a way to reduce stack size, as it is expensive for GC.

Note: This implementation implicitly defines the semantics of Terminable to be as follows. A middleware that implements Terminable *must* always delegate to the next layer. If Terminable is not implemented, it will be wrapped in a StackedHttpKernel that handles the terminate() delegation. The delegation *must not* be conditional.

A consequence of this is that all middlewares will be called every time, each one exactly once.
@igorw igorw closed this as completed in 17887ec Nov 23, 2014
igorw added a commit that referenced this issue Nov 23, 2014
@igorw
Copy link
Contributor

igorw commented Nov 23, 2014

This issue has been fixed in v1.0.3, thanks so much for reporting it!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants