[
https://issues.apache.org/jira/browse/TS-3453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14573478#comment-14573478
]
ASF GitHub Bot commented on TS-3453:
------------------------------------
GitHub user shinrich opened a pull request:
https://github.com/apache/trafficserver/pull/212
TS-3453: Cleaning up handling SSL events in write_to_net_io
This is a tidy up. It is possible that unnecessarily removing things from
ready lists might be causing problems. Please review to make sure I'm not
missing something obvious here.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shinrich/trafficserver ts-3453
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/212.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #212
----
commit 42e9ebbc26cd539ecf50ce1328324bffb4f143e2
Author: shinrich <[email protected]>
Date: 2015-06-04T19:55:05Z
TS-3453: Cleaning up handling SSL events in write_to_net_io
----
> Confusion of handling SSL events in write_to_net_io in UnixNetVConnection.cc
> ----------------------------------------------------------------------------
>
> Key: TS-3453
> URL: https://issues.apache.org/jira/browse/TS-3453
> Project: Traffic Server
> Issue Type: Bug
> Components: SSL
> Reporter: Susan Hinrichs
> Assignee: Susan Hinrichs
> Fix For: 6.0.0
>
>
> While tracking down differences for SSL between 5.0 and 5.2 for TS-3451 I
> came across odd event handling code in write_to_net_io in
> UnixNetVConnection.cc. Looking back at the history in that code things
> became even more confusing.
> The current version on master (same as what is in 5.2) contains the following
> in an if/else sequence. SSL IOs can return READ events event from write
> functions (and visa versa), so that is why I assume that this write function
> is dealing with SSL read events at all.
> {code}
> if (ret == SSL_HANDSHAKE_WANT_READ || ret == SSL_HANDSHAKE_WANT_ACCEPT ||
> ret == SSL_HANDSHAKE_WANT_CONNECT
> || ret == SSL_HANDSHAKE_WANT_WRITE) {
> vc->read.triggered = 0;
> nh->read_ready_list.remove(vc);
> vc->write.triggered = 0;
> nh->write_ready_list.remove(vc);
> if (ret == SSL_HANDSHAKE_WANT_READ || ret == SSL_HANDSHAKE_WANT_ACCEPT)
> read_reschedule(nh, vc);
> else
> write_reschedule(nh, vc);
> }
> {code}
> Seems odd to be clearing and remove read events if the real event was only a
> write. And visa versa, seems odd to be clearing and removing write events if
> the real event was a read.
> It seems to me that the sequence should be replaced with something like
> {code}
> if (ret == SSL_HANDSHAKE_WANT_READ || ret == SSL_HANDSHAKE_WANT_ACCEPT) {
> vc->read.triggered = 0;
> nh->read_ready_list.remove(vc);
> read_reschedule(nh, vc);
> } else if (ret == SSL_HANDSHAKE_WANT_CONNECT || ret ==
> SSL_HANDSHAKE_WANT_WRITE) {
> vc->write.triggered = 0;
> nh->write_ready_list.remove(vc);
> write_reschedule(nh, vc);
> }
> {code}
> Looking back at the history shows adding and removing and re-adding of
> reschedules.
> * TS-3006 9/22/14 by me. Adds in the read_reschedule case
> * TS-2815 5/16/14 by [~bcall] Removes the read_reschedule case
> * TS-2211 10/28/13 by postwait Adds read_reschedule and protects the
> write_reschedule and read_reschedule with specific event checks.
> * TS-1921 5/17/13 by [[email protected]] Adds in the write_reschedule
> This seems like an obvious tidy up thing. I'm not addressing a specific
> issue here, but the current thing seems wrong. Given the history, I'm
> hesitant to clean things up without review from those that came before.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
