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
@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.

@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
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.

None yet

4 participants