Re: [mongrel2] filter for configurable error pages
- From:
- Jubaraj Borgohain
- Date:
- 2012-01-30 @ 01:57
Hi Dalton,
Thank you for taking the time to review the filter. I agree with your
analysis and have updated the filter. Please take a look here
https://github.com/jubarajborgohain/mongrel2/commit/7c1019e58f358f7d0b3d6a4be0631b1b8d890d1c
.
Please keep the comments coming, :)
Thanks,
Juba
On Sun, Jan 29, 2012 at 2:25 PM, Dalton Barreto <daltonmatos@gmail.com>wrote:
> 2012/1/20 Jubaraj Borgohain <jubarajborgohain@gmail.com>:
> > I just finished a draft version of a filter that would allow a user to
> serve
> > custom error pages.
> >
> > I would appreciate it would if you would review it:
> >
>
https://github.com/jubarajborgohain/mongrel2/commit/73785145c5d687320501565c15f28dd8f999e2c5
> >
>
> Hello Jubaraj,
>
> Firstly, sorry for the long delay to reply to your email.
>
> Although I couldn't run your code locally (yet!) I have a simple
> observation:
>
> When you build the response headers, you have to stat() the custom
> errorpage file to know its size. But the code does not check
> to see if the size calculation went well, so it's possible do return a
> "Content-lenght: -1" header to the client.
>
> The code also opens the file (to read its contents) after building the
> response headers. Maybe it's a good idea do first open the file and
> only
> then build the full response?
>
> If the file_size() fails (returning -1) probably the open() will also
> fail, but at this point the response headers were already built and
> IOBuf_send() were already called.
> When open() fails the code will jump to "error:" and this will make
> the filter return "next_state" (that's the same state received by the
> filter), telling mongrel2 to continue to process the filter chain but
> we already
> returned the request headers. Don't know what would happen in this case.
>
> What do you think?
>
> Thanks,
>
> --
> Dalton Barreto
> http://daltonmatos.com
>
Re: [mongrel2] filter for configurable error pages
- From:
- Dalton Barreto
- Date:
- 2012-01-31 @ 01:47
2012/1/29 Jubaraj Borgohain <jubarajborgohain@gmail.com>:
> Hi Dalton,
>
> Thank you for taking the time to review the filter. I agree with your
> analysis and have updated the filter. Please take a look here
>
https://github.com/jubarajborgohain/mongrel2/commit/7c1019e58f358f7d0b3d6a4be0631b1b8d890d1c.
>
> Please keep the comments coming, :)
>
Hello Jubaraj,
I just tested your filter locally and it works perfectly! But I got a
little lost at the beginning until I realized that the filter only
register itself for
DIRECTORY routes, and I was trying to test it with a non-existent handler =).
Does it make sense to return custom error pages also for not found
handlers? Or mongrel2 does not even give the chance to catch this
situation?
Other than this, it works very well! Congratulations!
p.s: If others could also review the code it would be awesome, but for
me it's good to be merged.
Thanks,
--
Dalton Barreto
http://daltonmatos.com
Re: [mongrel2] filter for configurable error pages
- From:
- Jubaraj Borgohain
- Date:
- 2012-01-31 @ 15:18
Thank you for testing and running it. I was looking through the code that
is run after the state is set to HTTP_REQ and it seems like there are
checks for errors that may occur if a handler is not found or if the
handler is down.
One issue that I see in this "error page filter" is how a valid request
will be processed. In this case m2 would be doing redundant checks, once in
the filter and again in m2 core. In order to avoid this do you think it
would be nice to have a HTTP_ERROR_RESP state? The error response will only
be sent to the client in this state. Another advantage I see from having a
state like this would be the ability to chain two filters together. If
someone wrote a "access filter" and set the state to HTTP_ERROR_RESP and
error code to 403, we could handle it in the "error page filter" and return
the appropriate error page.
Since I am only beginning to gain familiarity with the m2 code base, there
is a high probability that what I am proposing is already in m2. If so
please disregard this.
Juba
On Jan 30, 2012 7:48 PM, "Dalton Barreto" <daltonmatos@gmail.com> wrote:
> 2012/1/29 Jubaraj Borgohain <jubarajborgohain@gmail.com>:
> > Hi Dalton,
> >
> > Thank you for taking the time to review the filter. I agree with your
> > analysis and have updated the filter. Please take a look here
> >
>
https://github.com/jubarajborgohain/mongrel2/commit/7c1019e58f358f7d0b3d6a4be0631b1b8d890d1c
> .
> >
> > Please keep the comments coming, :)
> >
>
> Hello Jubaraj,
>
> I just tested your filter locally and it works perfectly! But I got a
> little lost at the beginning until I realized that the filter only
> register itself for
> DIRECTORY routes, and I was trying to test it with a non-existent handler
> =).
>
> Does it make sense to return custom error pages also for not found
> handlers? Or mongrel2 does not even give the chance to catch this
> situation?
>
> Other than this, it works very well! Congratulations!
>
> p.s: If others could also review the code it would be awesome, but for
> me it's good to be merged.
>
>
> Thanks,
>
> --
> Dalton Barreto
> http://daltonmatos.com
>
Re: [mongrel2] filter for configurable error pages
- From:
- Dalton Barreto
- Date:
- 2012-01-31 @ 17:55
2012/1/31 Jubaraj Borgohain <jubarajborgohain@gmail.com>:
> Thank you for testing and running it. I was looking through the code that is
> run after the state is set to HTTP_REQ and it seems like there are checks
> for errors that may occur if a handler is not found or if the handler is
> down.
>
> One issue that I see in this "error page filter" is how a valid request will
> be processed. In this case m2 would be doing redundant checks, once in the
> filter and again in m2 core.
Yes, I also thought about this when reading the code and I saw that the filter
calculates if the file will return a 200 or 404 status.
> In order to avoid this do you think it would be
> nice to have a HTTP_ERROR_RESP state? The error response will only be sent
> to the client in this state. Another advantage I see from having a state
> like this would be the ability to chain two filters together. If someone
> wrote a "access filter" and set the state to HTTP_ERROR_RESP and error code
> to 403, we could handle it in the "error page filter" and return the
> appropriate error page.
>
> Since I am only beginning to gain familiarity with the m2 code base, there
> is a high probability that what I am proposing is already in m2. If so
> please disregard this.
>
I think this is a great idea! But maybe the best to do is to open a
new thread with this new states proposal.
Thinking quickly, m2 could have this HTTP_ERROR_RESP state and also
some internal error codes. For example for a Dir route it could be 404
or 403 and for a handler route there could be, for example,
HANDLER_NOT_FOUND or HANDLER_DOWN or even HANDLER_WOULD_BOCK (Don't
even know if it's possible so ask zeromq if a send() would block or
not, but if yes, it would be awesome to have a filter that spawn new
handlers on this situation).
But since this would require modifications on the m2 state machine a
new thread would be more appropriate, so Zed and others that have more
familiarity with this part of the code could give their thoughts.
What do you think?
--
Dalton Barreto
http://daltonmatos.com
Re: [mongrel2] filter for configurable error pages
- From:
- Jubaraj Borgohain
- Date:
- 2012-01-31 @ 18:23
Thanks for the response. I will start a new thread for HTTP_ERROR_RESP
state proposal.
On Tue, Jan 31, 2012 at 11:55 AM, Dalton Barreto <daltonmatos@gmail.com>wrote:
> 2012/1/31 Jubaraj Borgohain <jubarajborgohain@gmail.com>:
> > Thank you for testing and running it. I was looking through the code
> that is
> > run after the state is set to HTTP_REQ and it seems like there are checks
> > for errors that may occur if a handler is not found or if the handler is
> > down.
> >
> > One issue that I see in this "error page filter" is how a valid request
> will
> > be processed. In this case m2 would be doing redundant checks, once in
> the
> > filter and again in m2 core.
>
> Yes, I also thought about this when reading the code and I saw that the
> filter
> calculates if the file will return a 200 or 404 status.
>
> > In order to avoid this do you think it would be
> > nice to have a HTTP_ERROR_RESP state? The error response will only be
> sent
> > to the client in this state. Another advantage I see from having a state
> > like this would be the ability to chain two filters together. If someone
> > wrote a "access filter" and set the state to HTTP_ERROR_RESP and error
> code
> > to 403, we could handle it in the "error page filter" and return the
> > appropriate error page.
> >
> > Since I am only beginning to gain familiarity with the m2 code base,
> there
> > is a high probability that what I am proposing is already in m2. If so
> > please disregard this.
> >
>
> I think this is a great idea! But maybe the best to do is to open a
> new thread with this new states proposal.
>
> Thinking quickly, m2 could have this HTTP_ERROR_RESP state and also
> some internal error codes. For example for a Dir route it could be 404
> or 403 and for a handler route there could be, for example,
> HANDLER_NOT_FOUND or HANDLER_DOWN or even HANDLER_WOULD_BOCK (Don't
> even know if it's possible so ask zeromq if a send() would block or
> not, but if yes, it would be awesome to have a filter that spawn new
> handlers on this situation).
>
> But since this would require modifications on the m2 state machine a
> new thread would be more appropriate, so Zed and others that have more
> familiarity with this part of the code could give their thoughts.
>
> What do you think?
>
> --
> Dalton Barreto
> http://daltonmatos.com
>
--
Juba
*@jubaborgohain <http://www.twitter.com/jubaborgohain>*