You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by bu...@apache.org on 2013/03/20 01:11:19 UTC
[Bug 54728] New: HttpServletRequest#upgrade() and other interfaces
don't match spec; change may be impossible to implement
https://issues.apache.org/bugzilla/show_bug.cgi?id=54728
Bug ID: 54728
Summary: HttpServletRequest#upgrade() and other interfaces
don't match spec; change may be impossible to
implement
Product: Tomcat 8
Version: trunk
Hardware: All
OS: All
Status: NEW
Severity: critical
Priority: P2
Component: Catalina
Assignee: dev@tomcat.apache.org
Reporter: nicholas@nicholaswilliams.net
Classification: Unclassified
Created attachment 30081
--> https://issues.apache.org/bugzilla/attachment.cgi?id=30081&action=edit
Patch that doesn't work #1
More information in http://tomcat.markmail.org/thread/wsvablpkzagyrg6g.
Spec change discussed here:
http://java.net/projects/servlet-spec/lists/jsr340-experts/archive/2012-11/message/9
As discussed in Servlet spec mailing list, Shing's idea was:
<quote>
And we will require that the class passed in has a no ops constructor. In this
case, the user can configure the HttpUpgradeHandler instance before upgrade
processing and will be benefited by dependency injection.
</quote>
In the Proposed Final Draft of the spec, this was further defined:
<quote>
When an upgrade request is received, the servlet can invoke the
HttpServletRequest.upgrade method, which starts the upgrade process. This
method instantiates the given HttpUpgradeHandler class. The returned
HttpUpgradeHandler instance may be further customized. The application prepares
and sends an appropriate response to the client. After exiting the service
method of the servlet, the servlet container completes the processing of all
filters and marks the connection to be handled by the HttpUpgradeHandler. It
then calls the HttpUpgradeHandler's init method, passing a WebConnection to
allow the protocol handler access to the data streams.
</quote>
There are some fundamental problems here, and this may need to be escalated to
the spec level (in my opinion, this change was a significant DISimprovement of
the spec):
1) In Tomcat, specifically, this change shakes the foundation of how WebSockets
has been implemented. The HttpUpgradeHandler's init() method is supposed to be
called AFTER the Servlet's service() method returns. But how? Who is supposed
to call this? The constructor for AbstractProcessor is the only thing currently
calling init(). I tried to dive into how to refactor that, and my eyes started
bleeding. It goes deep into event dispatching in the Nio, Bio and Ajp
processors. I don't see a clean way (I don't see ANY way) to change this. At
the very least, someone with MUCH more Tomcat expertise than I will have to
tackle that.
2) The original purpose of the upgrade() method, supposedly, is to upgrade the
request. But, according to the paragraph above, the only purpose of the
upgrade() method now is to construct an instance of the class you pass into it
and return that instance...as if you couldn't do that more easily by simply
using the constructor. It sounds like the request doesn't ACTUALLY get upgraded
until LATER.
What it boils down to is that you need a fully-wired HttpUpgradeHandler in
order to actually upgrade the request, so either init() cannot be called from
upgrade() (in which case the upgrade() method is now pointless, IMO), or an
already-wired instance of HttpUpgradeHandler (not a class) needs to be passed
in to upgrade, like it was before.
I took two approaches to this. The first (obvious) one was to create an
doAfterUpgrade() method on WsUpgradeHandler to be called by WsServlet after it
calls upgrade(). This method took the parameters originally passed to the
constructor. Unfortunately, by the time execution got to the doAfterUpgrade()
method, the socket was already trying to read/write data and it had no endpoint
to do it to. NullPointerExceptions abounded (in
o.a.coyote.http11.upgrade.AbstractProcessor#upgradeDispatch()). It compiles,
but it doesn't upgrade. I've attached that patch.
I then tried a trick to achieve wiring-before-instantiation using a local
class, but Java was too smart for me. It replaced my no-arg constructor with an
arg constructor, and it couldn't be instantiated. I'll attach that patch in a
comment to follow this up.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 54728] HttpServletRequest#upgrade() and other interfaces don't
match spec; change may be impossible to implement
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54728
--- Comment #1 from Nick Williams <ni...@nicholaswilliams.net> ---
Created attachment 30082
--> https://issues.apache.org/bugzilla/attachment.cgi?id=30082&action=edit
Patch that doesn't work #2
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 54728] HttpServletRequest#upgrade() and other interfaces don't
match spec; change may be impossible to implement
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54728
Mark Thomas <ma...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
--- Comment #2 from Mark Thomas <ma...@apache.org> ---
This has been fixed in trunk.
Your first patch isn't far off. I dropped o.a.catalina.websocket as the old
implementation needed to be dropped at some point and now was a good a time as
any.
The other differences were:
- leave init() as is
- move most of the constructor to preInit() (similar to your doAfterUpgrade())
- call preInit() from the Servlet immediately after calling
HttpServletRequest.upgrade()
The actual upgrade is performed by the container once the Servlet.service()
method exits. The upgrade() call doesn't trigger the upgrade it just tells the
container which object is to handler the connection once the current HTTP
request completes.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 54728] HttpServletRequest#upgrade() and other interfaces don't
match spec; change may be impossible to implement
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54728
Nick Williams <ni...@nicholaswilliams.net> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #30081|0 |1
is patch| |
Attachment #30081|application/octet-stream |text/plain
mime type| |
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 54728] HttpServletRequest#upgrade() and other interfaces don't
match spec; change may be impossible to implement
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54728
--- Comment #3 from Nick Williams <ni...@nicholaswilliams.net> ---
Okay, I see where my mistake was. Wow. *rubs eyes*
I think I need to go back to bed. Lol.
Thanks.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org