Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

Fix log.tail by calling add after listening for events #882

Merged
merged 1 commit into from
Nov 26, 2018

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Oct 30, 2018

This got broken in go-ipfs 0.4.18, I have no idea why it worked before..

@ghost ghost assigned magik6k Oct 30, 2018
@ghost ghost added the in progress label Oct 30, 2018
@achingbrain
Copy link
Collaborator

There are some linting failures:

  1. Strings should use single quotes
  2. The commit message should not be sentence case so I think fix: Fix log.tail.. needs changing to be fix: fix log.tail

@magik6k magik6k force-pushed the fix/logtail-add-in-test branch 3 times, most recently from 03eea7f to 7543c06 Compare October 30, 2018 20:53
@daviddias daviddias force-pushed the fix/logtail-add-in-test branch from 7543c06 to 278cd06 Compare November 3, 2018 08:54
@ghost ghost assigned daviddias Nov 3, 2018
@daviddias
Copy link
Contributor

@magik6k rebased master onto your branch so that it runs with latest go-ipfs.

Note, I didn't see it fail on master.

test/log.spec.js Outdated
const req = ipfs.log.tail((err, res) => {
clearInterval(i)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I'm mis-interpreting what is happening here. My understanding of this is:

  1. We setup an interval to run once every second
  2. Call ipfs.log.tail - which will callback almost immediately with a stream for us to consume
  3. Clear the interval - presumably before the first time it runs (unless ipfs.log.tail takes > 1s to callback)

So I think adding this interval is either ineffectual, or it's relying on ipfs.log.tail to take > 1s to callback.

If this test is passing then it might be a false positive, which we need to get to the bottom of.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this should be in res.once callback.. It was quite late when I wrote this fix.

Looked at this more - with ipfs daemon --offline, doing curl http://127.0.0.1:5001/api/v0/log/tail -v, go-ipfs will not send response headers until first log message, so this is why this worked.

This test also works currently because go-ipfs tends to report some random events, but there is no guarantee that they will happen and this causes the test to hang for way too long.

@magik6k magik6k force-pushed the fix/logtail-add-in-test branch from 278cd06 to 61d3f59 Compare November 16, 2018 05:32
@alanshaw
Copy link
Contributor

Merging as test failures are for ping tests - unrelated to this PR.

@alanshaw alanshaw merged commit da35b0f into master Nov 26, 2018
@alanshaw alanshaw deleted the fix/logtail-add-in-test branch November 26, 2018 11:08
@ghost ghost removed the in progress label Nov 26, 2018
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants