raindrops RubyGem user+dev discussion/patches/pulls/bugs/help
 help / Atom feed
* [PATCH 1/2] Add setup and teardown for ipv6 tests
@ 2013-09-09 19:03 Hleb Valoshka
  2013-09-09 19:03 ` [PATCH 2/2] Return real stats instead of True Hleb Valoshka
  2014-11-14  3:15 ` [PATCH 1/2] Add setup and teardown for ipv6 tests Eric Wong
  0 siblings, 2 replies; 10+ messages in thread
From: Hleb Valoshka @ 2013-09-09 19:03 UTC (permalink / raw)
  To: raindrops

---
 test/test_linux_ipv6.rb | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/test/test_linux_ipv6.rb b/test/test_linux_ipv6.rb
index ec08f28..9e8730a 100644
--- a/test/test_linux_ipv6.rb
+++ b/test/test_linux_ipv6.rb
@@ -12,6 +12,14 @@ class TestLinuxIPv6 < Test::Unit::TestCase
 
   TEST_ADDR = ENV["TEST_HOST6"] || "::1"
 
+  def setup
+    @to_close = []
+  end
+
+  def teardown
+    @to_close.each { |io| io.close unless io.closed? }
+  end
+
   def test_tcp
     s = TCPServer.new(TEST_ADDR, 0)
     port = s.addr[1]
-- 
1.8.4



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] Return real stats instead of True
  2013-09-09 19:03 [PATCH 1/2] Add setup and teardown for ipv6 tests Hleb Valoshka
@ 2013-09-09 19:03 ` Hleb Valoshka
  2013-09-09 19:20   ` Eric Wong
  2014-11-14  3:15 ` [PATCH 1/2] Add setup and teardown for ipv6 tests Eric Wong
  1 sibling, 1 reply; 10+ messages in thread
From: Hleb Valoshka @ 2013-09-09 19:03 UTC (permalink / raw)
  To: raindrops

---
 ext/raindrops/linux_inet_diag.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ext/raindrops/linux_inet_diag.c b/ext/raindrops/linux_inet_diag.c
index cd4a876..bae1202 100644
--- a/ext/raindrops/linux_inet_diag.c
+++ b/ext/raindrops/linux_inet_diag.c
@@ -620,11 +620,9 @@ static VALUE tcp_listener_stats(int argc, VALUE *argv, VALUE self)
 			return rv;
 		}
 		for (i = 0; i < len; i++) {
-			union any_addr check;
 			VALUE cur = rb_ary_entry(addrs, i);
 
-			parse_addr(&check, cur);
-			rb_hash_aset(rv, cur, Qtrue);
+			rb_hash_aset(rv, cur, tcp_stats(&args, cur));
 		}
 		/* fall through */
 	}
-- 
1.8.4



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Return real stats instead of True
  2013-09-09 19:03 ` [PATCH 2/2] Return real stats instead of True Hleb Valoshka
@ 2013-09-09 19:20   ` Eric Wong
  2013-09-09 19:37     ` Hleb Valoshka
  2016-02-25  9:54     ` Eric Wong
  0 siblings, 2 replies; 10+ messages in thread
From: Eric Wong @ 2013-09-09 19:20 UTC (permalink / raw)
  To: raindrops

Hleb Valoshka <375gnu@gmail.com> wrote:

<no commit message body>

This deserves an explanation (and ideally, a test case).  I haven't
looked at the code in a while and needed to think a bit about
when the fix is relevant.

Thanks.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Return real stats instead of True
  2013-09-09 19:20   ` Eric Wong
@ 2013-09-09 19:37     ` Hleb Valoshka
  2013-09-09 19:44       ` Eric Wong
  2016-02-25  9:54     ` Eric Wong
  1 sibling, 1 reply; 10+ messages in thread
From: Hleb Valoshka @ 2013-09-09 19:37 UTC (permalink / raw)
  To: raindrops

On 9/9/13, Eric Wong <normalperson@yhbt.net> wrote:
> This deserves an explanation (and ideally, a test case).  I haven't
> looked at the code in a while and needed to think a bit about
> when the fix is relevant.

You better reject it. I've made it because of test failure:

Error: test_tcp_multi(TestLinuxIPv6)
  NoMethodError: undefined method `queued' for true:TrueClass
test/test_linux_ipv6.rb:54:in `test_tcp_multi'
     51:     addrs = [ addr1, addr2 ]
     52:     stats = tcp_listener_stats(addrs)
     53:     assert_equal 2, stats.size
  => 54:     assert_equal 0, stats[addr1].queued
     55:     assert_equal 0, stats[addr1].active
     56:     assert_equal 0, stats[addr2].queued
     57:     assert_equal 0, stats[addr2].active

With that changes test's passed. (I didn't noticed that time that you
process that Qtrues).

But the real reason may be something else: I've got IPv6 address using
Teredo and getnameinfo returns error with it. So it cause other
errors, I need to check it better.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Return real stats instead of True
  2013-09-09 19:37     ` Hleb Valoshka
@ 2013-09-09 19:44       ` Eric Wong
  2013-09-09 20:35         ` Hleb Valoshka
  0 siblings, 1 reply; 10+ messages in thread
From: Eric Wong @ 2013-09-09 19:44 UTC (permalink / raw)
  To: raindrops

Hleb Valoshka <375gnu@gmail.com> wrote:
> On 9/9/13, Eric Wong <normalperson@yhbt.net> wrote:
> > This deserves an explanation (and ideally, a test case).  I haven't
> > looked at the code in a while and needed to think a bit about
> > when the fix is relevant.
> 
> You better reject it. I've made it because of test failure:

Ah, thanks.  I've been forgetting to test on IPv6-enabled systems :x

(I typically run custom kernels without IPv6, there were
 compatibility/slowdown issues (way) back in the day and it was
 easier to just not enable IPv6 at all.
 Probably moot at this point...)

> But the real reason may be something else: I've got IPv6 address using
> Teredo and getnameinfo returns error with it. So it cause other
> errors, I need to check it better.

OK.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Return real stats instead of True
  2013-09-09 19:44       ` Eric Wong
@ 2013-09-09 20:35         ` Hleb Valoshka
  2013-09-09 22:03           ` Eric Wong
  0 siblings, 1 reply; 10+ messages in thread
From: Hleb Valoshka @ 2013-09-09 20:35 UTC (permalink / raw)
  To: raindrops

On 9/9/13, Eric Wong <normalperson@yhbt.net> wrote:

>> But the real reason may be something else: I've got IPv6 address using
>> Teredo and getnameinfo returns error with it. So it cause other
>> errors, I need to check it better.
> OK.

Using some debug output I've found the reason (it's not a teredo, it
was disabled).

Lets look into linux_inet_diag.c, if tcp_listener_stats has only 1 arg, you do

rb_hash_aset(rv, addrs, tcp_stats(&args, addrs));
(or cur = rb_ary_entry(addrs, 0);
rb_hash_aset(rv, cur, tcp_stats(&args, cur)); )
return rv;

But if it has more agrs, you do

    for (i = 0; i < len; i++) {
      union any_addr check;
      VALUE cur = rb_ary_entry(addrs, i);

      parse_addr(&check, cur);
      rb_hash_aset(rv, cur, Qtrue);

and then fallback to branch where arg is nil, the most interesting
thing is calling of st_AND_hash(key, stats, hash) which has

 if (rb_hash_lookup(hash, key) == Qtrue) {
      VALUE v = rb_listen_stats(stats);
      OBJ_FREEZE(key);
      rb_hash_aset(hash, key, v);
    }

The problem is `key'. It may differ from what you expect.

For example, in tests you do
TEST_ADDR = ENV["TEST_HOST6"] || "::1"
addr1, addr2 = "[#{TEST_ADDR}]:#{port1}", "[#{TEST_ADDR}]:#{port2}"

In my last case they are [::1]:42244, [::1]:46335, but values of `key'
for them are:
[::1%3336]:42244, [::1%3336]:46335, so rb_hash_lookup never returns Qtrue.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Return real stats instead of True
  2013-09-09 20:35         ` Hleb Valoshka
@ 2013-09-09 22:03           ` Eric Wong
  2013-09-10  7:18             ` Hleb Valoshka
  0 siblings, 1 reply; 10+ messages in thread
From: Eric Wong @ 2013-09-09 22:03 UTC (permalink / raw)
  To: raindrops

Thanks for the analysis.  Can you send a fix?  Otherwise I'll try to
take care of it within 2 weeks (going on vacation in a few days :)


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Return real stats instead of True
  2013-09-09 22:03           ` Eric Wong
@ 2013-09-10  7:18             ` Hleb Valoshka
  0 siblings, 0 replies; 10+ messages in thread
From: Hleb Valoshka @ 2013-09-10  7:18 UTC (permalink / raw)
  To: raindrops

On 9/10/13, Eric Wong <normalperson@yhbt.net> wrote:

> Thanks for the analysis.  Can you send a fix?  Otherwise I'll try to
> take care of it within 2 weeks (going on vacation in a few days :)

OK, I'll try


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] Add setup and teardown for ipv6 tests
  2013-09-09 19:03 [PATCH 1/2] Add setup and teardown for ipv6 tests Hleb Valoshka
  2013-09-09 19:03 ` [PATCH 2/2] Return real stats instead of True Hleb Valoshka
@ 2014-11-14  3:15 ` Eric Wong
  1 sibling, 0 replies; 10+ messages in thread
From: Eric Wong @ 2014-11-14  3:15 UTC (permalink / raw)
  To: raindrops

Oops, forgot this trivial patch.  Applied and pushed.
New release + mailing list coming soon.  Thanks.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Return real stats instead of True
  2013-09-09 19:20   ` Eric Wong
  2013-09-09 19:37     ` Hleb Valoshka
@ 2016-02-25  9:54     ` Eric Wong
  1 sibling, 0 replies; 10+ messages in thread
From: Eric Wong @ 2016-02-25  9:54 UTC (permalink / raw)
  To: Hleb Valoshka; +Cc: raindrops-public

Eric Wong <normalperson@yhbt.net> wrote:
> Hleb Valoshka <375gnu@gmail.com> wrote:
> 
> <no commit message body>
> 
> This deserves an explanation (and ideally, a test case).  I haven't
> looked at the code in a while and needed to think a bit about
> when the fix is relevant.

Resurrecting this from the old list 2.5 years ago:
http://bogomips.org/raindrops-public/20130909192020.GA18010@dcvr.yhbt.net/t/#u

I actually found a different reason for this problem.  Normally,
I never change the set of listeners.  But lately I've been
switching between backend HTTP servers that needed to run over TCP
(as opposed to Unix sockets) and dropping listeners occasionally
was causing "true" to show up in the results.

-----------8<------------
Subject: [PATCH] linux: tcp_listener_stats drops "true" placeholders

With invalid addresses specified which give no currently-bound
address, we must avoid leaving placeholders ('true' objects)
in our results.

Clean up some shadowing "cur" while we're at it.
---
 ext/raindrops/linux_inet_diag.c | 15 ++++++++++++---
 test/test_linux.rb              |  7 +++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/ext/raindrops/linux_inet_diag.c b/ext/raindrops/linux_inet_diag.c
index 35bb127..58415c6 100644
--- a/ext/raindrops/linux_inet_diag.c
+++ b/ext/raindrops/linux_inet_diag.c
@@ -618,6 +618,13 @@ static VALUE tcp_stats(struct nogvl_args *args, VALUE addr)
 	return rb_listen_stats(&args->stats);
 }
 
+static int drop_placeholders(st_data_t k, st_data_t v, st_data_t ign)
+{
+	if ((VALUE)v == Qtrue)
+		return ST_DELETE;
+	return ST_CONTINUE;
+}
+
 /*
  * call-seq:
  *      Raindrops::Linux.tcp_listener_stats([addrs[, sock]]) => hash
@@ -658,10 +665,9 @@ static VALUE tcp_listener_stats(int argc, VALUE *argv, VALUE self)
 	case T_ARRAY: {
 		long i;
 		long len = RARRAY_LEN(addrs);
-		VALUE cur;
 
 		if (len == 1) {
-			cur = rb_ary_entry(addrs, 0);
+			VALUE cur = rb_ary_entry(addrs, 0);
 
 			rb_hash_aset(rv, cur, tcp_stats(&args, cur));
 			return rv;
@@ -671,7 +677,7 @@ static VALUE tcp_listener_stats(int argc, VALUE *argv, VALUE self)
 			VALUE cur = rb_ary_entry(addrs, i);
 
 			parse_addr(&check, cur);
-			rb_hash_aset(rv, cur, Qtrue);
+			rb_hash_aset(rv, cur, Qtrue /* placeholder */);
 		}
 		/* fall through */
 	}
@@ -689,6 +695,9 @@ static VALUE tcp_listener_stats(int argc, VALUE *argv, VALUE self)
 	st_foreach(args.table, NIL_P(addrs) ? st_to_hash : st_AND_hash, rv);
 	st_free_table(args.table);
 
+	if (RHASH_SIZE(rv) > 1)
+		rb_hash_foreach(rv, drop_placeholders, Qfalse);
+
 	/* let GC deal with corner cases */
 	if (argc < 2) rb_io_close(sock);
 	return rv;
diff --git a/test/test_linux.rb b/test/test_linux.rb
index 0e79a86..bfefcc4 100644
--- a/test/test_linux.rb
+++ b/test/test_linux.rb
@@ -214,6 +214,13 @@ def test_tcp_multi
     assert_equal 0, stats[addr1].active
     assert_equal 1, stats[addr2].queued
     assert_equal 1, stats[addr2].active
+
+    # make sure we don't leave "true" placeholders in results if a
+    # listener becomes invalid (even momentarily).
+    s2.close
+    stats = tcp_listener_stats(addrs)
+    assert stats.values.all? { |x| x.instance_of?(Raindrops::ListenStats) },
+      "placeholders left: #{stats.inspect}"
   end
 
   # tries to overflow buffers
-- 
EW

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-09 19:03 [PATCH 1/2] Add setup and teardown for ipv6 tests Hleb Valoshka
2013-09-09 19:03 ` [PATCH 2/2] Return real stats instead of True Hleb Valoshka
2013-09-09 19:20   ` Eric Wong
2013-09-09 19:37     ` Hleb Valoshka
2013-09-09 19:44       ` Eric Wong
2013-09-09 20:35         ` Hleb Valoshka
2013-09-09 22:03           ` Eric Wong
2013-09-10  7:18             ` Hleb Valoshka
2016-02-25  9:54     ` Eric Wong
2014-11-14  3:15 ` [PATCH 1/2] Add setup and teardown for ipv6 tests Eric Wong

raindrops RubyGem user+dev discussion/patches/pulls/bugs/help

Archives are clonable:
	git clone --mirror https://bogomips.org/raindrops-public
	git clone --mirror http://ou63pmih66umazou.onion/raindrops-public

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.lang.ruby.raindrops
	nntp://ou63pmih66umazou.onion/inbox.comp.lang.ruby.raindrops

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox