Expiring of SelectionKeys should not be mandatory about grizzly HOT 14 CLOSED

• Issue Imported From: javaee/grizzly#21
• Original Issue Raised By:@glassfishrobot
• Original Issue Assigned To: @glassfishrobot
• Original Issue Closed By:@glassfishrobot

from grizzly.

@glassfishrobot Commented
Reported by [email protected]

from grizzly.

@glassfishrobot Commented
[email protected] said:
Added Fey Zheng to cc list.

from grizzly.

@glassfishrobot Commented
[email protected] said:
I think not have the timeout value is also bad, as it's potential DOS hole.
But it's possible to make timeout value pretty big.
For this reason we can reset Controller's SelectionKeyHandler.
Before starting controller we can do following:

DefaultSelectionKeyHandler handler = new DefaultSelectionKeyHandler();
handler.setTimeout(BIG_VALUE);
controller.setSelectionKeyHandler(handler);
.............continue with controller initialization & start ..............

Does it work for possible usecase?

from grizzly.

@glassfishrobot Commented
[email protected] said:
Actually, there's a little more going on here than I had first noticed.

The DefaultSelectionKeyHandler, Controller (default Controller) and
TCPSelectorHandler (the default SelectorHandler) all make the assumption that
the connections will need to use "keep alive" or some kind of timeout.

For the application Fey is working with, and GlassFish CORBA, the "keep alive"
is not needed nor desired. For this set of applications we do not need to
attach the current time to a SelectionKey when a new connection is established.
We do not need to iterate of the set of SelectionKey doing keep-alive work as
part of a Controller.doSelect(). And, the SelectionKeyHandler does not need to
know anything about an idle timeout or "keep alive".

I think what we need to do is some refactoring so that the
DefaultSelectionKeyHandler, the Controller and TCPSelectorHandler (along the
SSLSelectorHandler and UDPSelectorHandler) continue to work as they do today,
(i.e. we don't break existing applications already written that are using them).
Then, add a new SelectionKeyHandler and SelectorHandler(s) that do not use nor
need the "keep alive" functionality. This will also mean the Controller will
need to be updated since it also has the functionality built into its doSelect()
method.

However we get, I do not care. But, it is something we need to fix and we
should fix it pretty soon.

If we do it properly, we won't need a timeout for the set of applications that
do not need keep-alive. For those applications that do need a keep-alive, I
think we could keep the existing implementation as is.

from grizzly.

@glassfishrobot Commented
[email protected] said:
SelectionKey expiration code is moved from Controller to SelectionKeyHandler.
If this change solves the issue - SelectionKeyHandler.expire(SelectionKey, long)
should be removed from the SelectionKeyHandler interface.

from grizzly.

@glassfishrobot Commented
[email protected] said:
The potential problem with modifying or removing an existing interface is that
we may break existing applications that may be using it. Hence, I do not think
we can remove SelectionKeyHandler.expire(SelectionKey, long) from the
SelectionKeyHandler interface.

The fundamental issue here is that we do not have the flexibility to configure a
Grizzly transport in a manner that does not do "expiring of keys". This
includes (and not necessarily an exhaustive list):
1.) Controller.doSelect() should only do "expiring of keys" for those
applications that need it, (which should be the default). Applications that do
not need "expiring of keys" should be able to disable this functionality.
2.) The attaching of a Long (System.currentTimeMillis()) to a SelectionKey's
attachment should not be mandatory. Again, this should be the default since
"expiring keys" is the default configuration. But, when it is not desired, then
any attaching of a Long (System.currentTimeMillis()) should not be done at
"accept time", or in any other location.

In other words, we need to be able to have the default configuration for a
Grizzly transport that enables "expiring keys" and we need to be able to
configure a Grizzly transport that disables the "expiring keys" features and any
of its artifacts.

Perhaps this is justification for integrating a TransportConfiguration interface
/ object that allows one to configure a Grizzly transport more easily.

from grizzly.

@glassfishrobot Commented
[email protected] said:
I agree with both of you The goal of the SelectionKeyHandler is to give
control, for an application, of the SelectionKey life cycle (and a plus when you
compare to other framework). Looking at Alexey's changes, I would think we
should deprecate the expire(SelectionKey), and leave the new interface there.
That way the initial contract for the interface still meet the goal I was having
when designing it.

We should send a warning on the dev/user list to describe the interface changes.

from grizzly.

@glassfishrobot Commented
[email protected] said:
I will soon commit a tentative fix that remove all problems/dependencies Charlie
has found.

from grizzly.

@glassfishrobot Commented
[email protected] said:
Tentative fix:

Sending trunk/modules/grizzly/src/main/java/com/sun/grizzly/Controller.java
Sending
trunk/modules/grizzly/src/main/java/com/sun/grizzly/DefaultSelectionKeyHandler.java
Sending
trunk/modules/grizzly/src/main/java/com/sun/grizzly/SelectionKeyHandler.java
Sending
trunk/modules/grizzly/src/main/java/com/sun/grizzly/TCPSelectorHandler.java
Sending
trunk/modules/http/src/main/java/com/sun/grizzly/http/SelectorThread.java
Transmitting file data .....
Committed revision 425.

from grizzly.

@glassfishrobot Commented
[email protected] said:
Fix target

from grizzly.

@glassfishrobot Commented
Was assigned to [email protected]

from grizzly.

@glassfishrobot Commented
This issue was imported from java.net JIRA GRIZZLY-21

from grizzly.

@glassfishrobot Commented
Marked as fixed on Wednesday, December 16th 2009, 6:14:11 pm

from grizzly.