You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by shinrich <gi...@git.apache.org> on 2016/04/18 17:33:06 UTC
[GitHub] trafficserver pull request: TS-3429: Fix reference counting for TS...
GitHub user shinrich opened a pull request:
https://github.com/apache/trafficserver/pull/576
TS-3429: Fix reference counting for TSContScheduleEvery.
Changed the logic at the end of the INKContInternal event handler to bump up the ref-count at the end if it was invoked for an interval event. This enables the ref count to be incremented before every call to the event handler.
I ran with the Epic plugin (referenced by related bug), and its stats handler function executed multiple times without assertion failures.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shinrich/trafficserver ts-3429
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/576.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 #576
----
commit 3c1f2814e3524ca40e355d591f9f97fdd46d3b24
Author: shinrich <sh...@yahoo-inc.com>
Date: 2016-04-18T15:15:16Z
TS-3429: Fix reference counting for TSContScheduleEvery.
----
---
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, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3429: Fix reference counting for TS...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/576#issuecomment-211721466
I just feel like a lot can go wrong here, suppose I did a TSContCall w/ event = 2, that would cause event counts to increase and ultimately a memory leak...thoughts?
---
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, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3429: Fix reference counting for TS...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/576#issuecomment-211951906
Actually, that is why I added the check against period. If you do a schedule every, the handler gets called with event= EVENT_INTERVAL and e->period is non zero (positive I assume). In other cases, EVENT_INTERVAL can be sent, but the period will be 0.
---
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, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3429: Fix reference counting for TS...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/576#discussion_r60103328
--- Diff: proxy/InkAPI.cc ---
@@ -991,7 +991,14 @@ INKContInternal::handle_event(int event, void *edata)
Warning("INKCont Deletable but not deleted %d", m_event_count);
}
} else {
- return m_event_func((TSCont)this, (TSEvent)event, edata);
+ int retval = m_event_func((TSCont)this, (TSEvent)event, edata);
+ if (event == EVENT_INTERVAL) {
+ // In the interval case, we must re-increment the m_event_count for
+ // the next go around. Otherwise, our event count will go negative.
+ if (ink_atomic_increment((int *)&this->m_event_count, 1) < 0)
--- End diff --
I think this would be better as
```C
ink_release_assert(ink_atomic_increment((int *)&this->m_event_count, 1) < 0);
```
I agree this is a legitimate assertion; it is more helpful to have the actual assertion expression in the log.
---
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, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3429: Fix reference counting for TS...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/576#issuecomment-211506570
I'd like to review this a bit more ...
---
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, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3429: Fix reference counting for TS...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafficserver/pull/576
---
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, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3429: Fix reference counting for TS...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/576#issuecomment-214851707
@jpeach Any other thoughts here?
---
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, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3429: Fix reference counting for TS...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/576#discussion_r60242660
--- Diff: proxy/InkAPI.cc ---
@@ -991,7 +991,16 @@ INKContInternal::handle_event(int event, void *edata)
Warning("INKCont Deletable but not deleted %d", m_event_count);
}
} else {
- return m_event_func((TSCont)this, (TSEvent)event, edata);
+ int retval = m_event_func((TSCont)this, (TSEvent)event, edata);
+ if (edata && event == EVENT_INTERVAL) {
+ Event *e = reinterpret_cast<Event *>(edata);
+ if (e->period != 0) {
+ // In the interval case, we must re-increment the m_event_count for
+ // the next go around. Otherwise, our event count will go negative.
+ ink_release_assert(ink_atomic_increment((int *)&this->m_event_count, 1) >- 0);
--- End diff --
Yes, will fix.
---
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, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3429: Fix reference counting for TS...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/576#issuecomment-211473667
Looks good to me! I think this should fix at least two Jira's :).
---
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, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3429: Fix reference counting for TS...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/576#issuecomment-211571394
Agreed that this is snakey logic. Should take necessary time to review.
I just pushed a new version (will squash at the end). Changed the assert as suggested by James and then tracked down errors in the regress test because my original test was too broad.
I only changed one of the ink_asserts to a ink_release_assert. That pattern occurs numerous times in InkAPI.cc.
---
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, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3429: Fix reference counting for TS...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/576#issuecomment-214966094
This looks good to me too.
---
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, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3429: Fix reference counting for TS...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/576#issuecomment-211516035
I also have comments as I recently found another bug related to
TSContSchedulde/Every
Brian
On Tuesday, April 19, 2016, James Peach <no...@github.com> wrote:
> I'd like to review this a bit more ...
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/trafficserver/pull/576#issuecomment-211506570>
>
---
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, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3429: Fix reference counting for TS...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/576#discussion_r60170878
--- Diff: proxy/InkAPI.cc ---
@@ -991,7 +991,16 @@ INKContInternal::handle_event(int event, void *edata)
Warning("INKCont Deletable but not deleted %d", m_event_count);
}
} else {
- return m_event_func((TSCont)this, (TSEvent)event, edata);
+ int retval = m_event_func((TSCont)this, (TSEvent)event, edata);
+ if (edata && event == EVENT_INTERVAL) {
+ Event *e = reinterpret_cast<Event *>(edata);
+ if (e->period != 0) {
+ // In the interval case, we must re-increment the m_event_count for
+ // the next go around. Otherwise, our event count will go negative.
+ ink_release_assert(ink_atomic_increment((int *)&this->m_event_count, 1) >- 0);
--- End diff --
Pretty sure you mean >= 0.
---
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, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3429: Fix reference counting for TS...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/576#issuecomment-217931819
@PSUdaemon we should consider this for 6.x backport as we just ran into this issue in our 6.x deployment.
---
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, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3429: Fix reference counting for TS...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/576#issuecomment-212225694
@shinrich that makes sense.
---
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, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---