You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficserver.apache.org by calavera <gi...@git.apache.org> on 2016/09/29 00:57:36 UTC
[GitHub] trafficserver pull request #1062: [TS-4908] Remove duplicated closing contin...
GitHub user calavera opened a pull request:
https://github.com/apache/trafficserver/pull/1062
[TS-4908] Remove duplicated closing continuation.
The HTTP2Stream is closing the continuation twice when the transaction
is done. In debug mode, this causes a core dump because it tries to
close a closed continuation.
Signed-off-by: David Calavera <da...@gmail.com>
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/calavera/trafficserver remove_duplicated_closing_continuation
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/1062.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 #1062
----
commit c3819984943bd956f54e8f3dad5372f1c4035c47
Author: David Calavera <da...@gmail.com>
Date: 2016-09-29T00:52:43Z
[TS-4908] Remove duplicated closing continuation.
The HTTP2Stream is closing the continuation twice when the transaction
is done. In debug mode, this causes a core dump because it tries to
close a closed continuation.
Signed-off-by: David Calavera <da...@gmail.com>
----
---
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 #1062: [TS-4908] Remove duplicated cancelling eve...
Posted by shinrich <gi...@git.apache.org>.
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, 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 #1062: [TS-4908] Remove duplicated closing contin...
Posted by calavera <gi...@git.apache.org>.
Github user calavera commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1062#discussion_r81246655
--- Diff: proxy/http2/Http2Stream.cc ---
@@ -356,8 +356,6 @@ Http2Stream::transaction_done()
if (closed) {
// Safe to initiate SSN_CLOSE if this is the last stream
- if (cross_thread_event)
- cross_thread_event->cancel();
// Schedule the destroy to occur after we unwind here. IF we call directly, may delete with reference on the stack.
--- End diff --
Sorry, I agree that the title and description is misleading, I meant to say cancelling, not closing. Modifying title and description to be more correct.
---
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 issue #1062: [TS-4908] Remove duplicated closing continuation.
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1062
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/901/ for details.
---
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 issue #1062: [TS-4908] Remove duplicated cancelling action.
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1062
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/939/ for details.
---
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 issue #1062: [TS-4908] Remove duplicated cancelling action.
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/1062
[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, 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 #1062: [TS-4908] Remove duplicated cancelling act...
Posted by calavera <gi...@git.apache.org>.
Github user calavera commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1062#discussion_r81247196
--- Diff: proxy/http2/Http2Stream.cc ---
@@ -356,8 +356,6 @@ Http2Stream::transaction_done()
if (closed) {
// Safe to initiate SSN_CLOSE if this is the last stream
- if (cross_thread_event)
- cross_thread_event->cancel();
// Schedule the destroy to occur after we unwind here. IF we call directly, may delete with reference on the stack.
--- End diff --
If do_io_close set the event again, would it make sense to check if the even is cancelled rather than removing this code? In our tests, this doesn't happen, but I'm not familiar with the code to be sure it never happens.
---
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 issue #1062: [TS-4908] Remove duplicated cancelling event
Posted by shinrich <gi...@git.apache.org>.
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 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 issue #1062: [TS-4908] Remove duplicated cancelling event
Posted by calavera <gi...@git.apache.org>.
Github user calavera commented on the issue:
https://github.com/apache/trafficserver/pull/1062
@shinrich I made the change you suggested, making the even null and adding an assert replacing the double closing. I've also modified the title/description here and jira.
---
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 issue #1062: [TS-4908] Remove duplicated closing continuation.
Posted by masaori335 <gi...@git.apache.org>.
Github user masaori335 commented on the issue:
https://github.com/apache/trafficserver/pull/1062
[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, 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 issue #1062: [TS-4908] Remove duplicated closing continuation.
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1062
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/796/ for details.
---
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 issue #1062: [TS-4908] Remove duplicated cancelling action.
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1062
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/831/ for details.
---
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 #1062: [TS-4908] Remove duplicated closing contin...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1062#discussion_r81245130
--- Diff: proxy/http2/Http2Stream.cc ---
@@ -356,8 +356,6 @@ Http2Stream::transaction_done()
if (closed) {
// Safe to initiate SSN_CLOSE if this is the last stream
- if (cross_thread_event)
- cross_thread_event->cancel();
// Schedule the destroy to occur after we unwind here. IF we call directly, may delete with reference on the stack.
--- End diff --
This seems like a reasonable cleanup unless the earlier do_io_close could cause the cross_thread_event to be set again. NULLing out cross_thread_event earlier and adding an assert here would be reasonable.
However this fix doesn't seem to match the title. Canceling the event is not going to cause the close to happen twice.
---
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.
---