Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixed nntp Message Posting #1

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants

Hello,
I've been experimenting with your node-nntp library and I was running into issues with posting messages to a server. After spending a lot of time going through your code and debugging things, it seemed as though it had to do with the way that the send queue was working. What I observed is that I would queue up a message to post with the POST command, but it would include the message callback function in the same object when put on the queue. When it executed this, it would do the POST first but then shift the entire instruction off of the queue. When the app received the 340 - Ok to post message and then tried to send up the message, it was already shifted off of the queue and the message wouldn't get posted.

My fix was to add a condition, and if it was a post instruction, don't shift it off the queue until we receive the 340 response and actually post the message.

I"m fairly new to Node.js so my solution may cause issues with multiple requests coming in, but it does work to successfully post a message now. If you have any feedback, or would like me to make any other changes then I'd love to hear it. Or if there was no actual issue and I was missing something initially, then please let me know what I was doing wrong with posting messages.

Thanks!
Kevin

ksornberger added some commits Jun 8, 2011

Didn't have any success posting a message to usenet with original lib…
…rary, I've stepped through things and fixed what I think the issue was. Seems to successfully post messages now. A lot of debug messages still in the code though, I'll remove those soon.

Sorry about all of the debug message additions and deletions, those aren't part of the changes. This is my first pull request, and just realized I probably shouldn't have included them in the request changeset.

The main fix is just in the send function on lines 539 to 544

Owner

mscdex commented Aug 16, 2011

I know I'm a bit late to the game here, but what if instead you change line 126 of master from:

var cur = self._queue[0];

to:

var cur = self._curReq;

?

I think that should fix it. Either way let me know.

I haven't worked on this a little but, so I'll have to go back and double check what the issue was and if that will fix it. If it will, then that sounds good

Owner

mscdex commented Dec 14, 2012

This should be fixed in master since the rewrite. Open a new issue if you should happen to find that this is not the case.

@mscdex mscdex closed this Dec 14, 2012

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