[ https://issues.apache.org/jira/browse/TS-3453?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Susan Hinrichs updated TS-3453: ------------------------------- Description: 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 [~jpe...@apache.org] 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. was: 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 [~jpe...@apache.org] 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. > 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 > > 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 [~jpe...@apache.org] 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)