hi Andreas,
On Tue, Aug 25, 2009 at 2:13 AM, Andreas Veithen
<andreas.veit...@gmail.com>wrote:
> On Mon, Aug 24, 2009 at 10:33, Amila
> Suriarachchi<amilasuriarach...@gmail.com> wrote:
> >
> >
> > On Mon, Aug 24, 2009 at 12:44 PM, Andreas Veithen
> > <andreas.veit...@gmail.com> wrote:
> >>
> >> Amila,
> >>
> >> If you use your own TransportListener implementations, how do you
> >> implement getEPRsForService such that it returns URIs with the correct
> >> port numbers?
> >
> > See here[1]. I think this is not relevant here. What I try to say is any
> one
> > can implement a Transport listener in the way they need. We need to
> provide
> > the support for that.
>
> It definitely makes sense to allow people to use any TransportListener
> that extends AxisServletListener. Supporting any TransportListener,
> including classes that don't extend AxisServletListener, maybe makes
> sense from the perspective of Carbon (given the way it has been
> implemented), but from a pure Axis2 point of view, this is artificial.
> Probably if the issue in AxisServlet had been fixed before the
> development of Carbon, nobody would ever have suggested this.
May be. But the reality is carbon transport listeners are developed before
this change and
Axis2 1.0 has done nearly 3 years ago. So we need to support writing custom
transportListners without
extending AxisServletListner which was there before your change.
You have not put any comment in my suggestion to resolve the issue keeping
your change and with out breaking carbon. We will test it and commit if you
don't have any suggestions to fix this issue.
>
> > And also I think this code is not necessary as well.
> >
> > httpListener = getAxisServletListener(Constants.TRANSPORT_HTTP);
> > httpsListener =
> > getAxisServletListener(Constants.TRANSPORT_HTTPS);
> >
> > if (httpListener == null && httpsListener == null) {
> > log.warn("No transportReceiver for " +
> > AxisServletListener.class.getName() +
> > " found. An instance for HTTP will be configured
> > automatically. " +
> > "Please update your axis2.xml file!");
> > httpListener = new AxisServletListener();
> > TransportInDescription transportInDescription = new
> > TransportInDescription(
> > Constants.TRANSPORT_HTTP);
> > transportInDescription.setReceiver(httpListener);
> > axisConfiguration.addTransportIn(transportInDescription);
> >
> > } else if (httpListener != null && httpsListener != null
> > && httpListener.getPort() == -1 &&
> > httpsListener.getPort() == -1) {
> > log.warn("If more than one transportReceiver for " +
> > AxisServletListener.class.getName() + " exists,
> then
> > all instances " +
> > "must be configured with a port number. WSDL
> > generation will be " +
> > "unreliable.");
> > }
>
> The first part of the code is meant to ease transition from Axis2 1.5
> to 1.6. It is not strictly required, if we add something to the
> release note warning the user about the fact that the behavior of
> AxisServlet has changed. Since the second part only outputs a warning
> is also not strictly required, but it improves the user experience
> because it immediately gives the user a hint that there is something
> wrong with the configuration.
>
> > if we go ahead with this approach we need to have two axis2.xml.
> > 1. shift with standalone distribution which has the SimpleHttp trnasport
> > listener
> > 2. shift with web distribution with new Axis2Servlet listeners.
>
> That is exactly the intention I had.
>
> > At run time, if the axis2.xml (which is inside the war file) does not
> > contain AxisServletListner, this means they have done their own listener
> > extending the transprotListner.
>
> If axis2.xml doesn't contain AxisServletListener, this can mean one of
> four things:
>
> * The user didn't upgrade axis2.xml from 1.5 to 1.6.
> * There is a misconfiguration.
> * The user has implemented his own transport receiver that he wants to
> use with AxisServlet (assuming that AxisServlet supports
> TransportListener implementations not derived from
> AxisServletListener).
> * The user has configured another HTTP implementation and actually
> doesn't want to use AxisServlet.
>
> > So let them handle it in that way. We don't
> > need to initialize new things.
> >
>
> Note that there is also another, more pragmatic option to solve all
> these issue. What we know is this:
>
> * Both Synapse and Carbon extend AxisServlet in a way that makes these
> extensions tightly coupled to the way AxisServlet works. This is not
> an issue in the way Synapse or Carbon have been designed, but due to
> the fact that AxisServlet failed to provide enough flexibility to
> extend it in a proper and robust way.
> * Since almost all members in AxisServlet are protected (and also not
> final), any subclass has complete access to the internals and may
> override any method. This means that any change (fix or extension) to
> AxisServlet is risky or in some cases even impossible.
>
> Probably the right conclusion would be to leave AxisServlet as it is,
> deprecate it and create a new implementation. Not from scratch
> obviously, but as a fork of the existing code with the following
> changes:
>
> * All internals of the new AxisServlet would be private (or final)
> with the exception of methods that have a well defined behavior and/or
> that clearly indicate how a subclass may override them.
> * The new AxisServlet would clearly specify that methods such as init
> or doXxx may only be overridden if the subclass calls the
> corresponding method in AxisServlet. In return, AxisServlet would
> provide extension points or configuration options that allow the
> subclass to achieve the required level of customization. E.g. (to
> satisfy a requirement of Synapse) it would allow the subclass to
> instruct it not to create and start a ListenerManager.
>
> Thoughts?
First of all lets finish this discussion by fixing the carbon break issue. I
think my suggestion would do this.
This discussion should involve Axis2-dev, carbon-dev, synapse-dev. So please
start a discussion by engaging all these communities in a different thread
with the correct heading.
Here is my though about this.
As I mentioned earlier these project has started long time ago. So there may
be lots of possible major improvements. So if you keep thinking in that way
you may endup with changing every thing :).
>From your earlier change you try to solve some real user issues. So IMHO
unless there is such clear advantage it is better not to go for these
changes which may imply a recursive break of differet projects.
thanks,
Amila.
>
>
> Andreas
>
> > thanks,
> > Amila.
> >
> > [1]
> >
> https://wso2.org/repos/wso2/trunk/carbon/org.wso2.carbon.core/src/main/java/org/wso2/carbon/core/transports/http/HttpTransportListener.java
> >>
> >>
> >> Andreas
> >>
> >> On Mon, Aug 24, 2009 at 08:56, Amila
> >> Suriarachchi<amilasuriarach...@gmail.com> wrote:
> >> > hi,
> >> >
> >> > I had time to go through your comment and I think what you suggest is
> >> > the
> >> > correct way to address the problem.
> >> > So +1 for that.
> >> >
> >> > Then I looked into the our code and found that we have also done the
> >> > same
> >> > technique with separate two
> >> > Transport listeners :).
> >> >
> >> > Because we have our own two transport listeners this code gives us
> >> > problems,
> >> >
> >> > // This method should not be part of the public API. In particular we
> >> > must
> >> > not allow subclasses
> >> > // to override this method because we don't make any guarantees as
> >> > to
> >> > when exactly this method
> >> > // is called.
> >> > private void preprocessRequest(HttpServletRequest req) throws
> >> > ServletException {
> >> > initContextRoot(req);
> >> >
> >> > AxisServletListener listener = req.isSecure() ? httpsListener
> :
> >> > httpListener;
> >> > if (listener == null) {
> >> > throw new ServletException(req.getScheme() + " is
> >> > forbidden");
> >> > } else {
> >> > // Autodetect the port number if necessary
> >> > if (listener.getPort() == -1) {
> >> > listener.setPort(req.getServerPort());
> >> > }
> >> > }
> >> > }
> >> >
> >> > Here you assume anyone use Axis2 as a library has used
> >> > AxisServletListner
> >> > and kept it as a private variable in Axis2Servlet.
> >> >
> >> > Instead of I would like to implement this method like this,
> >> >
> >> >
> >> > // This method should not be part of the public API. In particular we
> >> > must
> >> > not allow subclasses
> >> > // to override this method because we don't make any guarantees as
> >> > to
> >> > when exactly this method
> >> > // is called.
> >> > private void preprocessRequest(HttpServletRequest req) throws
> >> > ServletException {
> >> > initContextRoot(req);
> >> >
> >> > TransportInDescription transportInDescription =
> >> > req.isSecure() ?
> >> > this.axisConfiguration.getTransportIn(Constants.TRANSPORT_HTTPS) :
> >> >
> >> > this.axisConfiguration.getTransportIn(Constants.TRANSPORT_HTTP);
> >> > if (transportInDescription == null) {
> >> > throw new ServletException(req.getScheme() + " is
> >> > forbidden");
> >> > } else {
> >> > if (transportInDescription.getReceiver() instanceof
> >> > AxisServletListener) {
> >> > AxisServletListener listener = (AxisServletListener)
> >> > transportInDescription.getReceiver();
> >> > if (listener.getPort() == -1) {
> >> > listener.setPort(req.getServerPort());
> >> > }
> >> > }
> >> > }
> >> > }
> >> >
> >> > In this way we do not keep the Axis2Specific listener and take it from
> >> > the
> >> > axis configuration and check for port only if it is an Axis2Servlet
> >> > listener.
> >> >
> >> > WDYT?
> >> >
> >> > thanks,
> >> > Amila.
> >> >
> >> >
> >> >
> >> > On Sat, Aug 22, 2009 at 12:45 PM, Srinath Perera <hemap...@gmail.com>
> >> > wrote:
> >> >>
> >> >> > I am not sure whether clearly understand the difference between how
> >> >> > AxisServelet works and how SimpleHTTPServer works. In the case of
> >> >> > AxisServlet, we first start AxisServlet and then we start Axis2, so
> >> >> > when
> >> >> > we start Axis2 we already have HTTP listener running (and hence
> >> >> > TransportInDec). So once the system start we just create a
> >> >> > TransportInDec using servlet and set that as the HTTP TransportDec
> >> >> > (and
> >> >> > of course we replace what is in axis2.xml for HTTP with this
> >> >> > transportInDec, and we must do that). Whereas in SimpleHTTPServer,
> we
> >> >> > first start Axis2 and then we use transport in axis2.xml to
> configure
> >> >> > the system, so we use the tarnporInDec to configure the
> >> >> > SimpleHTTPServer.
> >> >>
> >> >> Yep, this is right. One reason it is there is that to allow both
> >> >> simple HTTP server and servlet share the same axis2.xml which is not
> >> >> required, rather a convenience. In a servlet case transport receiver
> >> >> is not initialized at all (I do not think it has changed).
> >> >>
> >> >> Thanks
> >> >> Srinath
> >> >>
> >> >> --
> >> >> ============================
> >> >> Srinath Perera, Ph.D.
> >> >> WSO2 Inc. http://wso2.com
> >> >> Blog: http://srinathsview.blogspot.com/
> >> >
> >> >
> >> >
> >> > --
> >> > Amila Suriarachchi
> >> > WSO2 Inc.
> >> > blog: http://amilachinthaka.blogspot.com/
> >> >
> >
> >
> >
> > --
> > Amila Suriarachchi
> > WSO2 Inc.
> > blog: http://amilachinthaka.blogspot.com/
> >
>
--
Amila Suriarachchi
WSO2 Inc.
blog: http://amilachinthaka.blogspot.com/
