[GitHub] trafficserver pull request #1310: TS-5022: resource leak fix CID 1368316

2017-01-09 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1310 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1310: TS-5022: resource leak fix CID 1368316

2017-01-09 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1310 Looks good. Also addresses Issue 1308. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have

[GitHub] trafficserver issue #1224: TS-5056 Implement nonrecoverable error mechanism

2017-01-06 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1224 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1226: TS-5022: Allow multiple client cert for ATS

2017-01-06 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1226 Looks Good! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver pull request #1226: TS-5022: Allow multiple client cert for AT...

2017-01-06 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1226 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver pull request #1262: TS-5092: ATS handling of too many concurre...

2017-01-06 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1262 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver pull request #1271: TS-5103: replace ua_raw_buffer_reader with...

2017-01-06 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1271 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #801: TS-4664: Fix crash by unifying event handlers for ...

2017-01-05 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/801 I'm actually holding off on this one. We are running our version of 5.3.x with this change, but there were a number of other changes as well. Not clear this one is needed. I plan to do

[GitHub] trafficserver pull request #1257: TS-5053 const char **argv passed to TSPlug...

2017-01-05 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1257 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver pull request #1260: TS-5093: Augment and fix crash in slow log

2017-01-05 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1260 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver pull request #1253: TS-5084 Make logcat follow file rotation

2017-01-05 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1253 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver pull request #1263: TS-5091: Crash if server session from glob...

2017-01-05 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1263 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver pull request #1092: TS-4948: Adjust code to prevent NULL varia...

2017-01-05 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1092 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1092: TS-4948: Adjust code to prevent NULL variable ref...

2017-01-05 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1092 Looks like someone else fixed this along the way. Closing. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project

[GitHub] trafficserver pull request #1226: TS-5022: Allow multiple client cert for AT...

2017-01-05 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1226#discussion_r94826573 --- Diff: proxy/http/HttpSM.cc --- @@ -4059,6 +4061,16 @@ HttpSM::do_remap_request(bool run_inline) pending_action = remap_action_handle

[GitHub] trafficserver pull request #1226: TS-5022: Allow multiple client cert for AT...

2017-01-05 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1226#discussion_r94825933 --- Diff: iocore/net/SSLNetVConnection.cc --- @@ -980,7 +980,20 @@ SSLNetVConnection::sslStartHandShake(int event, int ) case

[GitHub] trafficserver pull request #1226: TS-5022: Allow multiple client cert for AT...

2017-01-05 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1226#discussion_r94824973 --- Diff: iocore/net/SSLNetProcessor.cc --- @@ -76,6 +77,7 @@ SSLNetProcessor::start(int, size_t stacksize) SSLError("Can't initi

[GitHub] trafficserver pull request #1226: TS-5022: Allow multiple client cert for AT...

2017-01-05 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1226#discussion_r94824666 --- Diff: iocore/net/SSLNetProcessor.cc --- @@ -67,7 +67,8 @@ SSLNetProcessor::start(int, size_t stacksize) // Acquire a SSLConfigParams

[GitHub] trafficserver issue #1257: TS-5053 const char **argv passed to TSPluginInit ...

2017-01-04 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1257 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1248: TS-5070 Add configuration variables to set file p...

2017-01-04 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1248 The logic looks reasonable. The invocation of the OWNER privilege looks correct. I guess we could avoid that elevation if we did the chmod at the point of the file was created, but I

[GitHub] trafficserver issue #1248: TS-5070 Add configuration variables to set file p...

2017-01-04 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1248 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1253: TS-5084 Make logcat follow file rotation

2017-01-04 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1253 @danobi can you squash? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled

[GitHub] trafficserver pull request #1254: TS-5085 `posix_fadvise` is incorrectly use...

2017-01-04 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1254 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver pull request #1226: TS-5022: Allow multiple client cert for AT...

2017-01-04 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1226#discussion_r94605355 --- Diff: iocore/net/P_SSLNetProcessor.h --- @@ -63,6 +64,90 @@ struct SSLNetProcessor : public UnixNetProcessor { return client_ctx

[GitHub] trafficserver issue #1300: TS-5108-SSL requests might stall because enabled ...

2017-01-04 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1300 Looks good to me as well. Indeed, the reenable is useless where it is currently for production deployment. --- If your project is set up for it, you can reply to this email and have your

[GitHub] trafficserver issue #1300: TS-5108-SSL requests might stall because enabled ...

2017-01-04 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1300 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1262: TS-5092: ATS handling of too many concurrent stre...

2017-01-04 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1262 Pushed new commit (squashed) to address maskit's comments. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project

[GitHub] trafficserver pull request #1226: TS-5022: Allow multiple client cert for AT...

2017-01-04 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1226#discussion_r94591982 --- Diff: iocore/net/P_SSLNetProcessor.h --- @@ -63,6 +64,90 @@ struct SSLNetProcessor : public UnixNetProcessor { return client_ctx

[GitHub] trafficserver pull request #1259: TS-5095: IOBufferReader::read_avail adds t...

2017-01-04 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1259 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1271: TS-5103: replace ua_raw_buffer_reader with ua_buf...

2017-01-04 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1271 Yes, this looks like the correct change to me. We should be checking that there is more data on the ua_buffer_reader. In this case, there should always be data on the ua_raw_buffer_reader

[GitHub] trafficserver issue #1254: TS-5085 `posix_fadvise` is incorrectly used in tr...

2016-12-14 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1254 Looks good to me. Can you squash? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature

[GitHub] trafficserver issue #1257: TS-5053 const char **argv passed to TSPluginInit ...

2016-12-14 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1257 Looks reasonable. Should we also be checking that we aren't overrunning the 64 long argv array? --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] trafficserver pull request #1263: TS-5091: Crash if server session from glob...

2016-12-14 Thread shinrich
GitHub user shinrich opened a pull request: https://github.com/apache/trafficserver/pull/1263 TS-5091: Crash if server session from global pool is not alive The Jira contains a stack track we have seen in production. The problem is the migration fails, but we set the netvc to null

[GitHub] trafficserver pull request #1262: TS-5092: ATS handling of too many concurre...

2016-12-14 Thread shinrich
GitHub user shinrich opened a pull request: https://github.com/apache/trafficserver/pull/1262 TS-5092: ATS handling of too many concurrent streams too agressive This issue was identified while debugging new errors seen by an internal team after they enabled HTTP/2 in their client

[GitHub] trafficserver issue #1253: TS-5084 Make logcat follow file rotation

2016-12-14 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1253 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1253: TS-5084 Make logcat follow file rotation

2016-12-14 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1253 Logic looks good to me. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled

[GitHub] trafficserver issue #1259: TS-5095: IOBufferReader::read_avail adds to CPU u...

2016-12-14 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1259 Some details in the jira description. We noticed this when we moved to a newer compiler and started building against openssl 1.0.2. Somewhere in that change, the communication patterns

[GitHub] trafficserver pull request #1260: TS-5093: Augment and fix crash in slow log

2016-12-13 Thread shinrich
GitHub user shinrich opened a pull request: https://github.com/apache/trafficserver/pull/1260 TS-5093: Augment and fix crash in slow log You can merge this pull request into a Git repository by running: $ git pull https://github.com/shinrich/trafficserver ts-5093

[GitHub] trafficserver pull request #1259: TS-5095: IOBufferReader::read_avail adds t...

2016-12-13 Thread shinrich
GitHub user shinrich opened a pull request: https://github.com/apache/trafficserver/pull/1259 TS-5095: IOBufferReader::read_avail adds to CPU utilization You can merge this pull request into a Git repository by running: $ git pull https://github.com/shinrich/trafficserver ts

[GitHub] trafficserver issue #1226: TS-5022: Allow multiple client cert for ATS

2016-12-13 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1226 I think there still needs to be some locking around the CtxMap to safely reload. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub

[GitHub] trafficserver issue #1226: TS-5022: Allow multiple client cert for ATS

2016-11-28 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1226 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1232: TS-4938: Avoid crashes due to NULL vc dereference...

2016-11-28 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1232 Looks good to me. One minor comment, but it was an issue with the original change. So I think the backport is good to go. --- If your project is set up for it, you can reply

[GitHub] trafficserver pull request #1232: TS-4938: Avoid crashes due to NULL vc dere...

2016-11-28 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1232#discussion_r89840018 --- Diff: proxy/http/HttpTransact.cc --- @@ -558,7 +558,8 @@ HttpTransact::BadRequest(State *s) void HttpTransact::HandleBlindTunnel(State

[GitHub] trafficserver issue #1226: TS-5022: Allow multiple client cert for ATS

2016-11-18 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1226 I chatted with Persia about cleanly dealing with the config reload case. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well

[GitHub] trafficserver issue #1226: TS-5022: Allow multiple client cert for ATS

2016-11-18 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1226 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1131: TS-4322: Proposal: NetProfileSM

2016-10-28 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1131 At first glance this looks reasonable. It is a big change, but that logic could use some restructuring. Perhaps I can get you to sketch out the class hierarchy on a white board during

[GitHub] trafficserver pull request #1131: TS-4322: Proposal: NetProfileSM

2016-10-28 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1131#discussion_r85536202 --- Diff: iocore/net/SSLProfileSM.cc --- @@ -206,20 +275,11 @@ ssl_read_from_net(SSLNetVConnection *sslvc, EThread *lthread, int64_t

[GitHub] trafficserver pull request #1131: TS-4322: Proposal: NetProfileSM

2016-10-28 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1131#discussion_r85533912 --- Diff: iocore/net/Makefile.am --- @@ -73,13 +76,11 @@ libinknet_a_SOURCES = \ P_Socks.h \ P_SSLCertLookup.h \ P_SSLConfig.h

[GitHub] trafficserver pull request #1131: TS-4322: Proposal: NetProfileSM

2016-10-28 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1131#discussion_r85533386 --- Diff: iocore/net/I_NetVConnection.h --- @@ -541,6 +545,31 @@ class NetVConnection : public VConnection unsigned int attributes

[GitHub] trafficserver pull request #1153: 6.2 backport TS-4507 TS-4813 TS-4892

2016-10-26 Thread shinrich
GitHub user shinrich opened a pull request: https://github.com/apache/trafficserver/pull/1153 6.2 backport TS-4507 TS-4813 TS-4892 You can merge this pull request into a Git repository by running: $ git pull https://github.com/shinrich/trafficserver 6-2-back-ts-4507-ts-4813

[GitHub] trafficserver pull request #1140: TS-5006: Fix CID 1356975

2016-10-26 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1140#discussion_r85231582 --- Diff: proxy/http/HttpSM.cc --- @@ -3309,7 +3309,7 @@ HttpSM::tunnel_handler_ua(int event, HttpTunnelConsumer *c) // only external

[GitHub] trafficserver pull request #1113: TS-4974: Bad debug assert in HttpSM::handl...

2016-10-20 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1113 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver pull request #1111: TS-4972: Configure collapsed_forwarding pl...

2016-10-20 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/ --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1117: TS-4916 Add safety net to avoid H2-infinite-loop ...

2016-10-18 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1117 Looks good to me as well. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled

[GitHub] trafficserver issue #1109: TS-4970: Crash in INKVConnInternal when handle_ev...

2016-10-18 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1109 Not a complete solution as Thomas noted, but may be sufficient to keep 6.2.x moving. Alternatively, we may just want to back port TS-4590. --- If your project is set up for it, you can

[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...

2016-10-17 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1024 Looks good. Persia addressed Leif and James' comments. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does

[GitHub] trafficserver pull request #1024: TS-4858: fix memory leak of global_default...

2016-10-17 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1024 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...

2016-10-17 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1024 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...

2016-10-14 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1024 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...

2016-10-14 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1024 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1109: TS-4970: Crash in INKVConnInternal when handle_ev...

2016-10-14 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1109 Would it be easier to just back port the fix from TS-4590? I'm a bit concerned about the proposed fix in this PR. I don't think is correctly using the m_deleted/m_deletable parameters

[GitHub] trafficserver pull request #1111: TS-4972: Configure collapsed_forwarding pl...

2016-10-14 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/#discussion_r83489086 --- Diff: plugins/experimental/collapsed_forwarding/collapsed_forwarding.cc --- @@ -265,49 +307,69 @@ collapsed_cont(TSCont contp, TSEvent event

[GitHub] trafficserver pull request #1113: TS-4974: Bad debug assert in HttpSM::handl...

2016-10-14 Thread shinrich
GitHub user shinrich opened a pull request: https://github.com/apache/trafficserver/pull/1113 TS-4974: Bad debug assert in HttpSM::handle_server_setup_error See jira for description. You can merge this pull request into a Git repository by running: $ git pull https

[GitHub] trafficserver pull request #1108: TS-4970: Crash in INKVConnInternal when ha...

2016-10-14 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1108#discussion_r83480249 --- Diff: proxy/InkAPI.cc --- @@ -1053,15 +1053,14 @@ int INKVConnInternal::handle_event(int event, void *edata

[GitHub] trafficserver pull request #1111: TS-4972: Configure collapsed_forwarding pl...

2016-10-14 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/#discussion_r83475325 --- Diff: plugins/experimental/collapsed_forwarding/collapsed_forwarding.cc --- @@ -265,49 +307,69 @@ collapsed_cont(TSCont contp, TSEvent event

[GitHub] trafficserver pull request #1111: TS-4972: Configure collapsed_forwarding pl...

2016-10-14 Thread shinrich
GitHub user shinrich opened a pull request: https://github.com/apache/trafficserver/pull/ TS-4972: Configure collapsed_forwarding plugin as global or remap. Add the ability to configure collapsed_forwarding plugin globally. Original version can only be configured per remap

[GitHub] trafficserver issue #1100: TS-4916: Fix for an H2-infinite-loop deadlock

2016-10-13 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1100 Not clear we want to move this to 7.0. The main changes have already been applied (check to see if stream is still in list before deleting and appropriately signal that the stream should

[GitHub] trafficserver pull request #1100: TS-4916: Fix for an H2-infinite-loop deadl...

2016-10-13 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1100#discussion_r83291135 --- Diff: proxy/http2/Http2ConnectionState.cc --- @@ -1097,6 +1141,7 @@ void Http2ConnectionState::send_data_frames(Http2Stream *stream

[GitHub] trafficserver pull request #1100: TS-4916: Fix for an H2-infinite-loop deadl...

2016-10-13 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1100#discussion_r83291283 --- Diff: proxy/http2/Http2Stream.cc --- @@ -267,10 +267,12 @@ Http2Stream::do_io_close(int /* flags */) // Make sure any trailing end

[GitHub] trafficserver pull request #1100: TS-4916: Fix for an H2-infinite-loop deadl...

2016-10-13 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1100#discussion_r83290881 --- Diff: proxy/http2/Http2ConnectionState.cc --- @@ -936,30 +940,70 @@ Http2ConnectionState::cleanup_streams() void Http2ConnectionState

[GitHub] trafficserver pull request #1088: TS-4915: Crash from hostdb in PriorityQueu...

2016-10-13 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1088 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1088: TS-4915: Crash from hostdb in PriorityQueueLess

2016-10-13 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1088 Removed the extra !empty() check in ::erase. The tests still pass. I'm running it in production again without incident. Will let it bake some more and then will merge it if all looks

[GitHub] trafficserver issue #1088: TS-4915: Crash from hostdb in PriorityQueueLess

2016-10-12 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1088 Squashed and rebased. Assuming all looks good with merge in the morning. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well

[GitHub] trafficserver pull request #1088: TS-4915: Crash from hostdb in PriorityQueu...

2016-10-12 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1088#discussion_r83086541 --- Diff: lib/ts/PriorityQueue.h --- @@ -110,6 +110,12 @@ PriorityQueue<T, Comp>::pop() return; } + // SKH - I s

[GitHub] trafficserver pull request #1088: TS-4915: Crash from hostdb in PriorityQueu...

2016-10-12 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1088#discussion_r83084441 --- Diff: lib/ts/PriorityQueue.h --- @@ -110,6 +110,12 @@ PriorityQueue<T, Comp>::pop() return; } + // SKH - I s

[GitHub] trafficserver issue #1088: TS-4915: Crash from hostdb in PriorityQueueLess

2016-10-12 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1088 @masaori335 ok, that makes sense. I reverted my hack in the _bubble_down. Hopefully that fixes that transient TSConfigSyntax build failure that occurred on the Linux build

[GitHub] trafficserver issue #1062: [TS-4908] Remove duplicated cancelling event

2016-10-11 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1062 Looks good to me. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled

[GitHub] trafficserver pull request #1062: [TS-4908] Remove duplicated cancelling eve...

2016-10-11 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1062 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1088: TS-4915: Crash from hostdb in PriorityQueueLess

2016-10-11 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1088 Bleah. Clang-formatted. The fix has been running without crash for nearly 24 hours on my prod box. --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] trafficserver issue #1088: TS-4915: Crash from hostdb in PriorityQueueLess

2016-10-11 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1088 Added a potential fix to _bubble_down to just do a simple bubble sort from the last index to the end of the array to catch any overshooting left index calculations. --- If your project

[GitHub] trafficserver issue #1088: TS-4915: Crash from hostdb in PriorityQueueLess

2016-10-11 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1088 The problem appears to be in _bubble_down. In the test, I had 4 items, w, x, y, z with weights 10, 20, 30, 40 respectively. I pushed in reverse order, but the overall order afterwards

[GitHub] trafficserver issue #1088: TS-4915: Crash from hostdb in PriorityQueueLess

2016-10-11 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1088 While writing the test, I see that the bubble_up/down isn't work (or at least isn't working as I expect). Was able to make a failing test without the erase fix. --- If your project

[GitHub] trafficserver pull request #1088: TS-4915: Crash from hostdb in PriorityQueu...

2016-10-11 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1088#discussion_r82863899 --- Diff: lib/ts/PriorityQueue.h --- @@ -110,6 +110,12 @@ PriorityQueue<T, Comp>::pop() return; } + // SKH - I s

[GitHub] trafficserver issue #1092: TS-4948: Adjust code to prevent NULL variable ref...

2016-10-11 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1092 I'm not very familiar with this code. Was just trying to adjust the logic to avoid the coverity found NULL error. We could adjust to send the traffic to the else in line 2179 which

[GitHub] trafficserver pull request #1092: TS-4948: Adjust code to prevent NULL varia...

2016-10-11 Thread shinrich
GitHub user shinrich opened a pull request: https://github.com/apache/trafficserver/pull/1092 TS-4948: Adjust code to prevent NULL variable reference. Attempting to fix a coverity warning by clearing the user_client_addr bool when the client_addr variable cannot be reset from NULL

[GitHub] trafficserver issue #1066: [TS-4457] Via header always reports http1

2016-10-11 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1066 The clean up changes were done and look good. Deeper review and approve done by @zwoop --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] trafficserver pull request #1066: [TS-4457] Via header always reports http1

2016-10-11 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1066 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver pull request #1088: TS-4915: Crash from hostdb in PriorityQueu...

2016-10-11 Thread shinrich
GitHub user shinrich opened a pull request: https://github.com/apache/trafficserver/pull/1088 TS-4915: Crash from hostdb in PriorityQueueLess These changes have been running on my production box since leaving work Monday night. Will keep an eye on it. Lower traffic overnight

[GitHub] trafficserver pull request #1083: TS-4938: Avoid crashes due to NULL vc dere...

2016-10-10 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1083 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1083: TS-4938: Avoid crashes due to NULL vc dereference...

2016-10-10 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1083 I've been running with this fix in production all day. No crashes on this issue. The push this morning addressed @bryancall and @jpeach comments. The most recent comment was just

[GitHub] trafficserver pull request #1024: TS-4858: fix memory leak of global_default...

2016-10-10 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1024#discussion_r82625741 --- Diff: iocore/net/SSLUtils.cc --- @@ -2159,7 +2123,7 @@ ssl_callback_session_ticket(SSL *ssl, unsigned char *keyname, unsigned char *iv

[GitHub] trafficserver pull request #1083: TS-4938: Avoid crashes due to NULL vc dere...

2016-10-10 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1083#discussion_r82613147 --- Diff: proxy/http/HttpTransact.cc --- @@ -568,7 +568,10 @@ HttpTransact::BadRequest(State *s) void HttpTransact::HandleBlindTunnel(State

[GitHub] trafficserver issue #1083: TS-4938: Avoid crashes due to NULL vc dereference...

2016-10-10 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1083 Probably don't need the server checks. I'll back those out. I've only seen ua_session->get_netvc() indirection crashes. Probably overgeneralized for the server side. --- If y

[GitHub] trafficserver pull request #1083: TS-4938: Avoid crashes due to NULL vc dere...

2016-10-06 Thread shinrich
GitHub user shinrich opened a pull request: https://github.com/apache/trafficserver/pull/1083 TS-4938: Avoid crashes due to NULL vc dereferences. While debugging the fix for TS-4813, I saw a crash due to a ua_session->get_netvc() being null be being dereferenced any

[GitHub] trafficserver issue #1024: TS-4858: fix memory leak of global_default_keyblo...

2016-10-05 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1024 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver pull request #1052: TS-4813: Fix lingering stream.

2016-10-05 Thread shinrich
Github user shinrich closed the pull request at: https://github.com/apache/trafficserver/pull/1052 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] trafficserver issue #1052: TS-4813: Fix lingering stream.

2016-10-05 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1052 @zwoop says docs has been running with this fix for 24 hours. I ran in production for a few hours. Had crashes due to TS-4915 and a new one due to TS-4938, but nothing related

[GitHub] trafficserver issue #1052: TS-4813: Fix lingering stream.

2016-10-05 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1052 Pushed up a squashed, tidied up branch. Running a build on this for a bit to make sure I didn't mess anything up in the tidy. --- If your project is set up for it, you can reply

[GitHub] trafficserver pull request #1075: TS-4905: Set parent NULL after destroy() i...

2016-10-05 Thread shinrich
Github user shinrich commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1075#discussion_r81987102 --- Diff: proxy/http/Http1ClientTransaction.cc --- @@ -67,6 +67,7 @@ Http1ClientTransaction::transaction_done() // If the parent session

[GitHub] trafficserver issue #1080: TS-4934: Remove invalid assert

2016-10-05 Thread shinrich
Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/1080 Looks good to me as well. In my working branch I had extended this assert to {code} // Check to see if the stream is in the closed state ink_assert(get_state

<    1   2   3   4   >