You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficserver.apache.org by oknet <gi...@git.apache.org> on 2016/08/27 12:18:55 UTC
[GitHub] trafficserver pull request #937: TS-2482: Fix Socks Proxy bugs
GitHub user oknet opened a pull request:
https://github.com/apache/trafficserver/pull/937
TS-2482: Fix Socks Proxy bugs
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/oknet/trafficserver TS-2482
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/937.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 #937
----
commit 91b08d549a9d23ad9165d0ad312c1dd2d43ce596
Author: Oknet Xu <xu...@skyguard.com.cn>
Date: 2016-08-27T12:18:06Z
TS-2482: Fix Socks Proxy bugs
----
---
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 #937: TS-2482: Should use target_addr instead of server_...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/937
@oknet FWIW if you need to assert unconditionally, you can use ``ink_abort``, which just aborts with a message.
---
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 #937: TS-2482: Should use target_addr instead of server_...
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/937
@zwoop ink_release_assert added.
---
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 #937: TS-2482: Should use target_addr instead of server_...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/937
:+1:
---
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 #937: TS-2482: Should use target_addr instead of server_...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/937
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/530/ 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 #937: TS-2482: Fix Socks Proxy bugs
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/937#discussion_r76537936
--- Diff: iocore/net/Socks.cc ---
@@ -136,7 +136,7 @@ void
SocksEntry::free()
{
MUTEX_TRY_LOCK(lock, action_.mutex, this_ethread());
- if (lock.is_locked()) {
+ if (!lock.is_locked()) {
// Socks continuation share the user's lock
// so acquiring a lock shouldn't fail
ink_assert(0);
--- End diff --
This reads a little strange, and has contradicting semantics in debug build (where it'll abort) vs release builds (where the call to SocksEntry::free() silently fails).
Would it make more sense to change this to an ink_release_assert()? E.g.
ink_release_assert(lock.is_locked);
? If not, then the invariant is not strong enough, and likely should be changed to not be an assert at all, right?
---
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 #937: TS-2482: Fix Socks Proxy bugs
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/937
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/633/ 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 #937: TS-2482: Should use target_addr instead of server_...
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/937
@jpeach It is conditional assert on lock check. Do u means keep the if statement and ink_abort inside it ?
---
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 #937: TS-2482: Should use target_addr instead of server_...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/937
@oknet the code is fine, it was just a FYI :)
---
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 #937: TS-2482: Fix Socks Proxy bugs
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/937
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/529/ 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 #937: TS-2482: Should use target_addr instead of server_...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/937
[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 #937: TS-2482: Fix Socks Proxy bugs
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/937
The changes 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 #937: TS-2482: Should use target_addr instead of server_...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/937
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/634/ 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 #937: TS-2482: Fix Socks Proxy bugs
Posted by mingzym <gi...@git.apache.org>.
Github user mingzym commented on the issue:
https://github.com/apache/trafficserver/pull/937
[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 #937: TS-2482: Should use target_addr instead of ...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach closed the pull request at:
https://github.com/apache/trafficserver/pull/937
---
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.
---