send: run callback in case of congestion #41 #43

Open
wants to merge 1 commit into
from

Projects

None yet

3 participants

@jbdemonte

No description provided.

@santigimeno
Collaborator

@jbdemonte tests don't pass with this change. Could you update them and / or add some new ones for the new functionality? Also, this could be considered a semver-major change. @bnoordhuis WDYT?

@jbdemonte

no problem, I will, I ran tests, I didn't notice error thrown (I usually use mocha :) )

@santigimeno
Collaborator

@jbdemonte there's no error thrown, but the test doesn't exit gracefully ( I used: npm test BTW)

@jbdemonte
jbdemonte commented Dec 14, 2016 edited

I updated the unit tests, let me know if it's ok on your side
For the major semver, I agree, but I let you do this choice

@jbdemonte

Is there any problem with my PR?

@bnoordhuis
Owner

I can't speak for @santigimeno but I probably won't have time to look at it this side of New Year's Eve.

@santigimeno
Collaborator

@jbdemonte it's on my radar but haven't had the time to look at it yet. I think I'll be able to throughout the week.

@jbdemonte
jbdemonte commented Dec 19, 2016 edited

oups... sorry for the bad click on "close the PR"

@jbdemonte jbdemonte closed this Dec 19, 2016
@jbdemonte jbdemonte reopened this Dec 19, 2016
@santigimeno

The changes look generally good to me. Thanks

+ if (err < 0) {
+ this.emit('error', errnoException(err, 'send'));
+ } else if (err === 1) {
+ this.emit('congestion', buf);
@santigimeno
santigimeno Dec 19, 2016 Collaborator

I think this should be this.emit('congestion');

@jbdemonte
jbdemonte Dec 20, 2016

Thanks to the buf parameter, you're able to resend it in the listeneer

@santigimeno
santigimeno Dec 20, 2016 Collaborator

Good idea

test/test-connect-callback.js
+
+var server = unix.createSocket('unix_dgram', function(buf, rinfo) {
+ console.error('server recv', '' + buf, arguments);
+ assert.equal('' + buf, 'PING' + seenCount);
@santigimeno
santigimeno Dec 19, 2016 Collaborator

I would remove all the logs from both files as now the tests are too noisy

@jbdemonte
jbdemonte Dec 20, 2016

I followed the other tests ;)

@santigimeno
santigimeno Dec 20, 2016 Collaborator

Yes, I know. It was already noisy, but now that we're sending 300 messages I think it's too much.

@jbdemonte
jbdemonte Dec 20, 2016

haha, for sure, I agree :D

test/test-connect-callback.js
+ /*
+ * Usually /proc/sys/net/unix/max_dgram_qlen is 10 so one that's being processed, 10 in the
+ * recv queue and the ... is dropped
+ */
@santigimeno
santigimeno Dec 19, 2016 Collaborator

Maybe add here that this only applies on linux?

@jbdemonte
jbdemonte Jan 3, 2017

To be honest, I copied this comment from the test-connect.js file, without having being able to see it in action

@santigimeno
santigimeno Jan 10, 2017 Collaborator

I know, I added it initially but we found out in #39 tht the behavior in Linux is different from OS X, also, the proc filesystem does not exist on OS X. So i think that the comment should be amended or just removed (and also on test-connect.js)

test/test-connect-callback.js
+ * Usually /proc/sys/net/unix/max_dgram_qlen is 10 so one that's being processed, 10 in the
+ * recv queue and the ... is dropped
+ */
+
@santigimeno
santigimeno Dec 19, 2016 Collaborator

remove extra line

test/test-connect-callback.js
+ }
+
+ send();
+
@santigimeno
santigimeno Dec 19, 2016 Collaborator

remove extra line

@jbdemonte

Sorry for the delay for your others comments, I didn't notice the requested changes
Btw, Happy New Year ;)

@jbdemonte

Do you need something else for this PR?

@santigimeno
Collaborator

@jbdemonte, sorry for the delay, I've been AFK most of the holiday season. I'll try yo look into this tomorrow. Thanks!

@santigimeno

There's still too many debug logs in both test files, I would remove them all, specially those that are going to be repeated 300 times.

test/test-connect-callback.js
+ /*
+ * Usually /proc/sys/net/unix/max_dgram_qlen is 10 so one that's being processed, 10 in the
+ * recv queue and the ... is dropped
+ */
@santigimeno
santigimeno Jan 10, 2017 Collaborator

I know, I added it initially but we found out in #39 tht the behavior in Linux is different from OS X, also, the proc filesystem does not exist on OS X. So i think that the comment should be amended or just removed (and also on test-connect.js)

@santigimeno
Collaborator

@jbdemonte can you rebase to current master and squash the PR into a single commit? Thanks!

@jbdemonte

It should be ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment