Skip to content
This repository was archived by the owner on Dec 31, 2025. It is now read-only.

inflight counting fixes#80

Open
tul wants to merge 4 commits intonats-io:masterfrom
tul:lkwd_dev
Open

inflight counting fixes#80
tul wants to merge 4 commits intonats-io:masterfrom
tul:lkwd_dev

Conversation

@tul
Copy link
Contributor

@tul tul commented Jan 25, 2018

Proposed fixes for issue #79

tul added 3 commits January 25, 2018 15:26
+ cache off nats connection to guard against it being nulled out by close
+ if it nats publish throws, remove the ack from the inflight map so it
  doesn't count against our limit forever
this unblocks any threads waiting within publish
+ previous behaviour was to block until inflight count was lower than max
+ can now control whether this blocking behaviour times out by setting
  Options.pubWait()
+ it still defaults to waiting forever
+ set pubWait to 0 to avoid blocking if max inflight is currently exceeded
@coveralls
Copy link

coveralls commented Jan 25, 2018

Coverage Status

Coverage decreased (-1.6%) to 91.324% when pulling 6dc8001 on mctully:lkwd_dev into 718bc88 on nats-io:master.

@sasbury
Copy link
Contributor

sasbury commented Aug 22, 2018

The code has chance significantly apparently, how would you like to proceed on this change?

@tul
Copy link
Contributor Author

tul commented Aug 22, 2018

Sorry, I haven’t yet looked at the new client lib release. When we move to the new library I will re-test for the original issue. If it still occurs then I’ll see whether I can update this patch. Thanks!

@tul
Copy link
Contributor Author

tul commented Aug 22, 2018

FWIW, I’ve just looked over the sources on master and I don’t think the areas I patched have changed significantly. I can see that publish() will still block indefinitely if max acks is hit, so I assume the original issue is still possible if you get in a state where acks are not popped.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants