Sign in Join

Live chat for Free Pascal, Lazarus, Delphi and every Pascal programmer.

Mailing lists / fpc-devel / 2010-07

Questions and suggestions to fpWeb

8 messages · 3 people · –

Reply by email Subscribe to this list Your subscription

Read-only archive. Use j and k to move between messages.

Luiz Americo Pereira Camara

Hi,

I've playing a bit with fpWeb and have some suggestions and questions:

- In TFPWebAction.DoHandleRequest if request is not handled by OnRequest and inherited, the content is copied to the response and handled is checked by the response content.
- FContensts will be always created even if is not created previously
- At this point AResponse.Contents can have some content?
- If so the handled checking is wrong. See these conditions

     AResponse.Contents.Text  '';
     Self.Contents.Text = '';
     Handled will be true. Should be false.

- Content and Contents properties are redundant. Also can lead to some performance issues. See the code of DoGetContent:

    If (Self.Content'') then
      Content.Write(Self.Content[1],Length(Self.Content));

Content is computed (Contents items concatenated) three times.

Don't be surprised if users do things like

 while  not ready do
  Content := Content + 'xxx';

- TFPWebAction.GetContent is empty. It seems that should call TFPWebAction.DoGetContent.
DoGetContent is inneficient. Cached FContents.Text and and isolated string references

- In TCustomFPWebModule.HandleRequest assigned(Session) will always evaluate to true (if Session is nil it will be created and then freed again).

- It's not necessary to check for nil in SetTemplate
- In the other side a check to nil should be added in SetContents to avoid creating FContents

- CGI apps enters in a infinite loop.
Adding

  if FWebHandler.FTerminated then
    Terminate;

to TCustomWebApplication.DoRun fixes it.

Follows patch with my suggestions.

Luiz

Reply by email

Michael Van Canneyt
to Luiz Americo Pereira Camara
Luiz Americo Pereira Camara wrote 13 lines
Hi, I've playing a bit with fpWeb and have some suggestions and questions: - In TFPWebAction.DoHandleRequest if request is not handled by OnRequest and inherited, the content is copied to the response and handled is checked by the response content. - FContensts will be always created even if is not created previously - At this point AResponse.Contents can have some content? - If so the handled checking is wrong. See these conditions AResponse.Contents.Text ''; Self.Contents.Text = ''; Handled will be true. Should be false.

Why ? It is handled ?

Show quoted text 14 lines
- Content and Contents properties are redundant. Also can lead to some performance issues. See the code of DoGetContent: If (Self.Content'') then Content.Write(Self.Content[1],Length(Self.Content)); Content is computed (Contents items concatenated) three times. Don't be surprised if users do things like while not ready do Content := Content + 'xxx';

I agree that the contents/content properties should be reduced to 1 property. But this is a change which will break backwards compatibility.

Show quoted text 5 lines
- TFPWebAction.GetContent is empty. It seems that should call TFPWebAction.DoGetContent. DoGetContent is inneficient. Cached FContents.Text and and isolated string references

Suggestions for improvements are welcome.

- In TCustomFPWebModule.HandleRequest assigned(Session) will always evaluate to true (if Session is nil it will be created and then freed again).

I will look into this.

Show quoted text 12 lines
- It's not necessary to check for nil in SetTemplate - In the other side a check to nil should be added in SetContents to avoid creating FContents - CGI apps enters in a infinite loop. Adding if FWebHandler.FTerminated then Terminate; to TCustomWebApplication.DoRun fixes it.

This is not sufficient, but I have committed a fix in revision 15698. Please test it.

Follows patch with my suggestions.

Can you please create bug reports for the remaining issues ?

Michael.

Reply by email

Marcos Douglas
to Michael Van Canneyt
Michael Van Canneyt wrote 19 lines
On Fri, 30 Jul 2010, Luiz Americo Pereira Camara wrote: [snip] > > - Content and Contents properties are redundant. Also can lead to some > performance issues. See the code of DoGetContent: > >  If (Self.Content'') then >    Content.Write(Self.Content[1],Length(Self.Content)); > > Content is computed (Contents items concatenated) three times. > > Don't be surprised if users do things like > > while  not ready do > Content := Content + 'xxx'; I agree that the contents/content properties should be reduced to 1 property. But this is a change which will break backwards compatibility.

So, start using the "deprecated" for Content property...

Regards,
MD

Reply by email

Luiz Americo Pereira Camara
to Michael Van Canneyt: Why ? It is handled ? I agree that the contents/content properties should be reduced to 1…
Michael Van Canneyt wrote 19 lines
On Fri, 30 Jul 2010, Luiz Americo Pereira Camara wrote: > Hi, > > I've playing a bit with fpWeb and have some suggestions and questions: > > - In TFPWebAction.DoHandleRequest if request is not handled by > OnRequest and inherited, the content is copied to the response and > handled is checked by the response content. > - FContensts will be always created even if is not created previously > - At this point AResponse.Contents can have some content? > - If so the handled checking is wrong. See these conditions > AResponse.Contents.Text ''; > Self.Contents.Text = ''; > Handled will be true. Should be false. Why ? It is handled ?

No. Because TWebAction.Contents is empty.

Show quoted text 15 lines
> - Content and Contents properties are redundant. Also can lead to > some performance issues. See the code of DoGetContent: > > If (Self.Content'') then > Content.Write(Self.Content[1],Length(Self.Content)); > > Content is computed (Contents items concatenated) three times. > > Don't be surprised if users do things like > > while not ready do > Content := Content + 'xxx'; I agree that the contents/content properties should be reduced to 1 property. But this is a change which will break backwards compatibility.

AFAIK the fpWeb interface is still in progress. Anyway, better change now than later.

Show quoted text 7 lines
> > - TFPWebAction.GetContent is empty. It seems that should call > TFPWebAction.DoGetContent. > DoGetContent is inneficient. Cached FContents.Text and and isolated > string references Suggestions for improvements are welcome.

See patch below (in previous mail)

Show quoted text 23 lines
> > - In TCustomFPWebModule.HandleRequest assigned(Session) will always > evaluate to true (if Session is nil it will be created and then freed > again). I will look into this. > > - It's not necessary to check for nil in SetTemplate > - In the other side a check to nil should be added in SetContents to > avoid creating FContents > > - CGI apps enters in a infinite loop. > Adding > > if FWebHandler.FTerminated then > Terminate; > > to TCustomWebApplication.DoRun fixes it. This is not sufficient, but I have committed a fix in revision 15698. Please test it.

OK. BTW what's the intention of adding TWebHandler AFAIK only add another layer.

Show quoted text 5 lines
> > Follows patch with my suggestions. Can you please create bug reports for the remaining issues ?

Done

Luiz

Reply by email

Michael Van Canneyt
to Luiz Americo Pereira Camara
Luiz Americo Pereira Camara wrote 22 lines
Michael Van Canneyt escreveu: > > > On Fri, 30 Jul 2010, Luiz Americo Pereira Camara wrote: > >> Hi, >> >> I've playing a bit with fpWeb and have some suggestions and questions: >> >> - In TFPWebAction.DoHandleRequest if request is not handled by OnRequest >> and inherited, the content is copied to the response and handled is >> checked by the response content. >> - FContensts will be always created even if is not created previously >> - At this point AResponse.Contents can have some content? >> - If so the handled checking is wrong. See these conditions >> AResponse.Contents.Text ''; >> Self.Contents.Text = ''; >> Handled will be true. Should be false. > > Why ? It is handled ? No. Because TWebAction.Contents is empty.

But that can be a perfectly valid result ?

Show quoted text 18 lines
>> - Content and Contents properties are redundant. Also can lead to some >> performance issues. See the code of DoGetContent: >> >> If (Self.Content'') then >> Content.Write(Self.Content[1],Length(Self.Content)); >> >> Content is computed (Contents items concatenated) three times. >> >> Don't be surprised if users do things like >> >> while not ready do >> Content := Content + 'xxx'; > > I agree that the contents/content properties should be reduced to 1 > property. But this is a change which will break backwards compatibility. AFAIK the fpWeb interface is still in progress. Anyway, better change now than later.

The question is: which one ?

Show quoted text 10 lines
>> >> - TFPWebAction.GetContent is empty. It seems that should call >> TFPWebAction.DoGetContent. >> DoGetContent is inneficient. Cached FContents.Text and and isolated string >> references > > Suggestions for improvements are welcome. See patch below (in previous mail)

I have seen it.

Show quoted text 27 lines
> >> >> - In TCustomFPWebModule.HandleRequest assigned(Session) will always >> evaluate to true (if Session is nil it will be created and then freed >> again). > > I will look into this. > >> >> - It's not necessary to check for nil in SetTemplate >> - In the other side a check to nil should be added in SetContents to avoid >> creating FContents >> >> - CGI apps enters in a infinite loop. >> Adding >> >> if FWebHandler.FTerminated then >> Terminate; >> >> to TCustomWebApplication.DoRun fixes it. > > This is not sufficient, but I have committed a fix in revision 15698. > Please test it. OK. BTW what's the intention of adding TWebHandler AFAIK only add another layer.

There is a simple reason: if you want to have a service application, there will be 2 application objects. Now you can have only 1 application object (service application) which uses the webhandler to process requests.

You can now also produce 1 binary which can be configured to run as a CGI/FCGI/Service/standalone server, and which will have 1 single application instance for all 4 cases.

Show quoted text 5 lines
>> Follows patch with my suggestions. > > Can you please create bug reports for the remaining issues ? Done

Thank you.

Michael.

Reply by email

Luiz Americo Pereira Camara
to Michael Van Canneyt
Michael Van Canneyt wrote 28 lines
On Tue, 3 Aug 2010, Luiz Americo Pereira Camara wrote: > Michael Van Canneyt escreveu: >> >> >> On Fri, 30 Jul 2010, Luiz Americo Pereira Camara wrote: >> >>> Hi, >>> >>> I've playing a bit with fpWeb and have some suggestions and questions: >>> >>> - In TFPWebAction.DoHandleRequest if request is not handled by >>> OnRequest and inherited, the content is copied to the response and >>> handled is checked by the response content. >>> - FContensts will be always created even if is not created previously >>> - At this point AResponse.Contents can have some content? >>> - If so the handled checking is wrong. See these conditions >>> AResponse.Contents.Text ''; >>> Self.Contents.Text = ''; >>> Handled will be true. Should be false. >> >> Why ? It is handled ? > > No. Because TWebAction.Contents is empty. But that can be a perfectly valid result ?

Yes. But in this case the appropriate way to mark as handled is to set Handled in OnRequest.

Moreover, how to leave Handled as false if even an empty Contents is treated as a value that affects the Handled state?

Show quoted text 21 lines
>>> - Content and Contents properties are redundant. Also can lead to >>> some performance issues. See the code of DoGetContent: >>> >>> If (Self.Content'') then >>> Content.Write(Self.Content[1],Length(Self.Content)); >>> >>> Content is computed (Contents items concatenated) three times. >>> >>> Don't be surprised if users do things like >>> >>> while not ready do >>> Content := Content + 'xxx'; >> >> I agree that the contents/content properties should be reduced to 1 >> property. But this is a change which will break backwards >> compatibility. > > AFAIK the fpWeb interface is still in progress. Anyway, better change > now than later. The question is: which one ?

Contents is more versatile and does not lead to performance pitfalls like using it as a simple AnsiString property

[..]

Show quoted text 13 lines
> > OK. BTW what's the intention of adding TWebHandler AFAIK only add > another layer. There is a simple reason: if you want to have a service application, there will be 2 application objects. Now you can have only 1 application object (service application) which uses the webhandler to process requests. You can now also produce 1 binary which can be configured to run as a CGI/FCGI/Service/standalone server, and which will have 1 single application instance for all 4 cases.

Good.

Do you have an simple example of such program?

Luiz

Reply by email

Michael Van Canneyt
to Luiz Americo Pereira Camara
Luiz Americo Pereira Camara wrote 35 lines
Michael Van Canneyt escreveu: > > > On Tue, 3 Aug 2010, Luiz Americo Pereira Camara wrote: > >> Michael Van Canneyt escreveu: >>> >>> >>> On Fri, 30 Jul 2010, Luiz Americo Pereira Camara wrote: >>> >>>> Hi, >>>> >>>> I've playing a bit with fpWeb and have some suggestions and questions: >>>> >>>> - In TFPWebAction.DoHandleRequest if request is not handled by OnRequest >>>> and inherited, the content is copied to the response and handled is >>>> checked by the response content. >>>> - FContensts will be always created even if is not created previously >>>> - At this point AResponse.Contents can have some content? >>>> - If so the handled checking is wrong. See these conditions >>>> AResponse.Contents.Text ''; >>>> Self.Contents.Text = ''; >>>> Handled will be true. Should be false. >>> >>> Why ? It is handled ? >> >> No. Because TWebAction.Contents is empty. > > But that can be a perfectly valid result ? Yes. But in this case the appropriate way to mark as handled is to set Handled in OnRequest. Moreover, how to leave Handled as false if even an empty Contents is treated as a value that affects the Handled state?

Ok, you have convinced me :-)

Show quoted text 24 lines
>>>> - Content and Contents properties are redundant. Also can lead to some >>>> performance issues. See the code of DoGetContent: >>>> >>>> If (Self.Content'') then >>>> Content.Write(Self.Content[1],Length(Self.Content)); >>>> >>>> Content is computed (Contents items concatenated) three times. >>>> >>>> Don't be surprised if users do things like >>>> >>>> while not ready do >>>> Content := Content + 'xxx'; >>> >>> I agree that the contents/content properties should be reduced to 1 >>> property. But this is a change which will break backwards compatibility. >> >> AFAIK the fpWeb interface is still in progress. Anyway, better change now >> than later. > > The question is: which one ? Contents is more versatile and does not lead to performance pitfalls like using it as a simple AnsiString property

So you prefer Contents ?
(that would be my preference)

Show quoted text 19 lines
[..] >> >> OK. BTW what's the intention of adding TWebHandler AFAIK only add another >> layer. > > There is a simple reason: if you want to have a service application, there > will be 2 application objects. Now you can have only 1 application object > (service application) which uses the webhandler to process requests. > > You can now also produce 1 binary which can be configured to run as a > CGI/FCGI/Service/standalone server, and which will have 1 single > application > instance for all 4 cases. Good. Do you have an simple example of such program?

Not yet for public use.

I personally need the case of a fastcgi service application, for use with ExternalFastCGIServer of mod_fastcgi. (*not* mod_fcgid, it has no such equivalent)

But I will try to cook up an application which does that.

Michael.

Reply by email

Luiz Americo Pereira Camara
to Michael Van Canneyt
Michael Van Canneyt wrote 14 lines
>>>> >>>> I agree that the contents/content properties should be reduced to 1 >>>> property. But this is a change which will break backwards >>>> compatibility. >>> >>> AFAIK the fpWeb interface is still in progress. Anyway, better >>> change now than later. >> >> The question is: which one ? > > Contents is more versatile and does not lead to performance pitfalls > like using it as a simple AnsiString property So you prefer Contents ?

Yes. In fact in the patch i sent i already removed Content.

Show quoted text 26 lines
>>> >>> OK. BTW what's the intention of adding TWebHandler AFAIK only add >>> another layer. >> >> There is a simple reason: if you want to have a service application, >> there >> will be 2 application objects. Now you can have only 1 application >> object >> (service application) which uses the webhandler to process requests. >> >> You can now also produce 1 binary which can be configured to run as a >> CGI/FCGI/Service/standalone server, and which will have 1 single >> application >> instance for all 4 cases. > > Good. > > Do you have an simple example of such program? Not yet for public use. I personally need the case of a fastcgi service application, for use with ExternalFastCGIServer of mod_fastcgi. (*not* mod_fcgid, it has no such equivalent) But I will try to cook up an application which does that.

Thanks. I'll be waiting.

Luiz

Reply by email