Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Weird error piping to >NUL #6039

Closed
LetItGlow opened this issue Mar 2, 2019 · 17 comments · Fixed by ipfs/go-ipfs-cmds#153
Closed

Weird error piping to >NUL #6039

LetItGlow opened this issue Mar 2, 2019 · 17 comments · Fixed by ipfs/go-ipfs-cmds#153
Assignees
Labels
kind/bug A bug in existing code (including security flaws) status/in-progress In progress topic/windows Windows specific

Comments

@LetItGlow
Copy link

LetItGlow commented Mar 2, 2019

In the last few days, I have written a batch, which encapsulates and "extends" some IPFS-commands.

As soon, I updated my binaries to 0.4.19, some parts of the batch broke.
For now what I have found, when I unpin a hash.

with 0.4.18, I could do this:

"%HOMEDRIVE%\command\%CURRENT_MODE%\ipfs" pin rm -r %_del.hash%>NUL 2>NUL
if /I '%errorlevel%'=='0' (
	echo Successfully unpinned
) else (
	echo %infostr% File not found
)

with 0.4.19, as soon there is a
>NUL
behind the command, it completely freaks out.
This:

"%HOMEDRIVE%\command\%CURRENT_MODE%\ipfs" pin rm -r %_del.hash%>NUL
if /I '%errorlevel%'=='0' (
	echo Successfully unpinned
) else (
	echo %infostr% File not found
)

Returns this:

Error: sync /dev/stdout: Unzulässige Funktion.
[ Info    ] File not found

It returns: Error: sync /dev/stdout: Unzulässige Funktion. and sets the Errorlevel to 1, although it successfully unpinned the given hash - verified with ipfs pin ls <hash> afterwards, so it's even more misleading.

When I change my code to this:

"%HOMEDRIVE%\command\%CURRENT_MODE%\ipfs" pin rm -r %_del.hash%
if /I '%errorlevel%'=='0' (
	echo Successfully unpinned
) else (
	echo %infostr% File not found
)

It returns:

unpinned <hash>
Successfully unpinned

Which is the expected output. I need the pape to >NUL though, as I don't need the output sometimes.
For now, I'll revert to 0.4.18

I am using Windows 10 x64, with the x64 binaries. Language: DE, if it matters.

@djdv
Copy link
Contributor

djdv commented Mar 4, 2019

There was a similar issue related to stderr here #6021
It looks like special handling will probably have to be put in place for NUL handling.
Sorry about that making it into the release. I need to build a more comprehensive set of Windows cases like this to test against. Pipes work, and even output to NUL (ipfs get -o NUL %HASH%)works, but this doesn't.

@Stebalien
Copy link
Member

@djdv can you add the appropriate error to https://github.com/ipfs/go-ipfs-cmds/error_windows.go (or maybe add some way to detect this?).

@Stebalien Stebalien added kind/bug A bug in existing code (including security flaws) topic/windows Windows specific labels Mar 4, 2019
djdv added a commit to djdv/go-ipfs-cmds that referenced this issue Mar 4, 2019
@ghost ghost added the status/in-progress In progress label Mar 4, 2019
@ghost ghost removed the status/in-progress In progress label Mar 4, 2019
@LetItGlow
Copy link
Author

Sorry for the late response and thank you for acknowledging this issue.
On Windows, >NUL redirects stdout to Null, combined as >NUL 2>NUL it even "blanks" stderr.
So, "DOS" piping works again with (hopefully soon) v0.4.20?

@djdv
Copy link
Contributor

djdv commented Mar 6, 2019

@LetItGlow
Yes, this should be resolved in the latest master.
If you would like to test a dev build (e375224) I have provided one here
[redacted - see comments below]
(obligatory random binary from the internet warning)

@LetItGlow
Copy link
Author

LetItGlow commented Mar 6, 2019

Heads up. Aside from this IPFS version executing incredible fast, it outputs flawless for now.

This:

rem %_del.hash% contains the hash without /ipfs/ or /ipns/ prefix
"%HOMEDRIVE%\command\%CURRENT_MODE%\ipfs" pin rm -r %_del.hash%>NUL 2>NUL
if /I '%errorlevel%'=='0' (
	for /F "tokens=2" %%a IN ('"%HOMEDRIVE%\command\%CURRENT_MODE%\ipfs" repo gc') DO (
		if /I '%%a'=='%_del.hash%' echo %infostr% File was successfully purged
	)
	if /I '%errorlevel%'=='0' (
		echo %infostr% Delete was successful
		echo %cautstr% It is not 100%% sure, this file will completely vanish from IPFS.
		echo %emptstr% If the content was pinned on another node, it may stay on IPFS.
	)
) else (
	rem echo what? Did pin rm fail?
	echo %infostr% File not found
)

goto End

used like this:

C:\Users\LetItGlow>ipfs1 add -r L:\IPFS_Storage\network_files\testsuite
 23.62 KiB / ? [---------------------------------------------------=---------] 
added QmetQyNZ8ifpujXAHAhDTf59FsPxSWMfNXApB6i6fveVa6 testsuite/favicon.ico
 24.84 KiB / 24.89 KiB [==============================================]  99.82%
added QmUw33rrDBfgrhB3dfBWbQUainP5863qo2Zjfan3CZTy88 testsuite/index.html
 24.89 KiB / 24.89 KiB [==============================================] 100.00%
added QmfDy98bRLsJQSQTnNjd2VSSKqQXsYxsyHEmGNFH9ZDsag testsuite/testme.txt
 24.89 KiB / 24.89 KiB [==============================================] 100.00%
added QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx testsuite
 24.89 KiB / 24.89 KiB [==============================================] 100.00%
C:\Users\LetItGlow>ipfs1 rm QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx
> Deleting file from IPFS...
[ Info    ] File was successfully purged
[ Info    ] Delete was successful
[ CAUTION ] It is not 100% sure, this file will completely vanish from IPFS.
            If the content was pinned on another node, it may stay on IPFS.
C:\Users\LetItGlow>ipfs1 rm QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx
> Deleting file from IPFS...
[ Info    ] File not found

Got the expected result. I gonna keep this dev-version for now, although I think I can't use it for bugreports, as it's an unofficial build?

@djdv
Copy link
Contributor

djdv commented Mar 6, 2019

Yay!
I believe the speedup (and likely memory reduction) is a result of building with Go 1.12. Currently, official releases are being built with 1.11.x since 1.12 hasn't been out very long yet (may contain bugs of its own).

Since this is from the master branch, and uses an official version of Go, bugs for it are still valid so long as you post the output of ipfs version --all with it.

@djdv
Copy link
Contributor

djdv commented Mar 6, 2019

@LetItGlow
My apologies, there's been a small mixup.
The build I gave you is actually commit 71d7cf7 but modified to include the fix (the only change is to use a later version of go-ipfs-cmds).
As a result it's not valid for bug reports.
I intended to send e375224
which I have put here
https://ipfs.io/ipfs/QmbbBXUv5shSs3QttaRipHFDMLT1GQmZggtHQbnfFYzkLH
This one is valid for bug reports.

For context, it appears like the build system is broken in a strange way.
For some reason binaries are being installed to %srcdir%\bin instead of %GOPATH%\bin which is how I provided an outdated binary.
My bad!

@LetItGlow
Copy link
Author

And my intuition was right. Should have noticed, when it said "71d7cf7XX"
let's see how e375224 turns out

@Kubuxu
Copy link
Member

Kubuxu commented Mar 6, 2019

@djdv I broke make install, fixing.

@ghost ghost assigned Kubuxu Mar 6, 2019
@ghost ghost added the status/in-progress In progress label Mar 6, 2019
@LetItGlow
Copy link
Author

LetItGlow commented Mar 6, 2019

71d7cf7 is better, e375224 has this bug in a different way.

rem try with "%HOMEDRIVE%\command\%CURRENT_MODE%\ipfs" pin rm -r %_del.hash%
C:\Users\LetItGlow>ipfs2 add -Q -r L:\IPFS_Storage\network_files\testsuite
QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx
C:\Users\LetItGlow>ipfs2 pin ls QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx
QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx recursive
C:\Users\LetItGlow>ipfs2 rm QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx
> Deleting file from IPFS...
unpinned QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx
[ Info    ] File was successfully purged
[ Info    ] Delete was successful
[ CAUTION ] It is not 100% sure, this file will completely vanish from IPFS.
            If the content was pinned on another node, it may stay on IPFS.

rem same test with 
rem "%HOMEDRIVE%\command\%CURRENT_MODE%\ipfs" pin rm -r %_del.hash%>NUL 2>NUL
C:\Users\LetItGlow>ipfs2 add -Q -r L:\IPFS_Storage\network_files\testsuite
QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx
C:\Users\LetItGlow>ipfs2 rm QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx
> Deleting file from IPFS...
[ Info    ] File not found

C:\Users\LetItGlow>ipfs2 version --all
go-ipfs version: 0.4.20-dev-e375224c3
Repo version: 7
System version: amd64/windows
Golang version: go1.12

edit: I am trying to get a cleaner version by making a standalone test scenario.

Ok, now I get it.

Start processing...
go-ipfs version: 0.4.20-dev-e375224c3
Repo version: 7
System version: amd64/windows
Golang version: go1.12
REPO is L:\IPFS_Storage\repo_2
CASE 1:
CASE 1 - IPFS add -Q -r "L:\IPFS_Storage\network_files\testsuite"
CASE 1: ADD (hash: QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx)
CASE 1 - pin ls OUTPUT:
QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx recursive
CASE 1 - pin ls OUTPUT END
CASE 1: pin rm -r QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx with NUL NUL
CASE 1: Found hash during REPO GC - OK.
CASE 1: Errorlevel is 0, success
CASE 1 - pin ls OUTPUT (VERIFICATION):
Error: path 'QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx' is not pinned
CASE 1: Expected: Found HASH during REPO, Errorlevel 0, not pinned.
=============================================================================
CASE 2:
CASE 2 - IPFS add -Q -r "L:\IPFS_Storage\network_files\testsuite"
CASE 2: ADD (hash: QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx)
CASE 2 - pin ls OUTPUT:
QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx recursive
CASE 2 - pin ls OUTPUT END
CASE 2: pin rm -r QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx without NUL NUL
unpinned QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx
CASE 2: Found hash during REPO GC - OK.
CASE 2: Errorlevel is 0, success
CASE 2 - pin ls OUTPUT (VERIFICATION):
Error: path 'QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx' is not pinned
CASE 2: Expected: Found HASH during REPO, Errorlevel 0, not pinned.

But while the daemon is active:

Start processing...
go-ipfs version: 0.4.20-dev-e375224c3
Repo version: 7
System version: amd64/windows
Golang version: go1.12
REPO is L:\IPFS_Storage\repo_2
CASE 1:
CASE 1 - IPFS add -Q -r "L:\IPFS_Storage\network_files\testsuite"
CASE 1: ADD (hash: QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx)
CASE 1 - pin ls OUTPUT:
QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx recursive
CASE 1 - pin ls OUTPUT END
CASE 1: pin rm -r QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx with NUL NUL
CASE 1: PIN RM failed.
CASE 1 - pin ls OUTPUT (VERIFICATION):
Error: path 'QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx' is not pinned
CASE 1: Expected: Found HASH during REPO, Errorlevel 0, not pinned.
=============================================================================
CASE 2:
CASE 2 - IPFS add -Q -r "L:\IPFS_Storage\network_files\testsuite"
CASE 2: ADD (hash: QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx)
CASE 2 - pin ls OUTPUT:
QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx recursive
CASE 2 - pin ls OUTPUT END
CASE 2: pin rm -r QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx without NUL NUL
unpinned QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx
CASE 2: Found hash during REPO GC - OK.
CASE 2: Errorlevel is 0, success
CASE 2 - pin ls OUTPUT (VERIFICATION):
Error: path 'QmXFeC3FsDQw3NcxFc89UrKeYryv3jnYvDsmmCceqeLXhx' is not pinned
CASE 2: Expected: Found HASH during REPO, Errorlevel 0, not pinned.

wat? as soon the daemon is running, using pipes results in a error, where the non-pipe works.
A short check with 71d7cf7 : both cases work with and without a running daemon, giving OK.

@djdv
Copy link
Contributor

djdv commented Mar 7, 2019

Sorry for the run around, we switched our dependency model recently.
This should work for real this time. 🤞

The problem with the last build was that it was not including the fixed version of the cmds lib and I must have tested it when the daemon was offline.

This is 3702368 except that it uses the latest cmds lib (now through go mod instead of through gx)
go get github.com/ipfs/go-ipfs-cmds@b21055ceb3d58e4630d4b0978ee97fb238013e4c

https://ipfs.io/ipfs/QmaD7s7mwwZkDRBQj5xSKFCVHWdDFuX5F5m6gjiUDLK7X2

This is a dirty build, but if you encounter issues with it that aren't already reported just link back to here in the issue. You can put the blame on me ;^)

Thanks for testing! I appreciate the patience with it.

@LetItGlow
Copy link
Author

I need to check ALL three versions again, because I stumbled on a funny problem, where deleting the "api" file while the daemon is running is a bad idea.

@LetItGlow
Copy link
Author

Case 1 is a test using pipes
Case 2 is a test with no pipes at all
I list the tests in the order I got the builds here.

4.18 (official):
Case 1&2 without daemon: OK
Case 1&2 with daemon: OK

4.19 (official):
Case 1&2 without daemon: OK
Case 1&2 with daemon: Case 1 (with pipes): FAILED, OK

0.4.20-dev-71d7cf7b0
Case 1&2 without daemon: OK
Case 1&2 with daemon: OK

0.4.20-dev-e375224c3
Case 1&2 without daemon: OK
Case 1&2 with daemon: Case 1 (with pipes): FAILED, OK

0.4.20-dev-3702368fe-dirty
Case 1&2 without daemon: OK
Case 1&2 with daemon: OK

While doing this, I stumbled on a problem.
When the daemon is shutdown incorrectly, it can leave the "api" file behind instead of deleting it.
IPFS complains with
Error: cannot connect to the api. Is the deamon running? To run as a standalone CLI command remove the api file in "$IPFS_PATH/api"

This also goes the other way, when a daemon is running, but the "api" file is deleted.
IPFS complains about writing a "repo.lock" (which is in use). This causes "pin rm -r" with no hash given to "get stuck".
I made that another testcase and in this error scenario, IPFS tries to read from stdin instead, where it gets no input inside a scripted event with no input pipe, getting perpetually stuck.

TL;DR:
0.4.20-dev-71d7cf7b0 and 0.4.20-dev-3702368fe-dirty
seem to work as intended - aside from another issue. Which one of those is the more recent?

@djdv
Copy link
Contributor

djdv commented Mar 7, 2019

Thanks @LetItGlow! Glad to hear it's working.
3702368 is the most recent commit.
It seems like we should treat the api file in a way similar to how we treat the repo lock files.
Ideally we'd hold exclusive deletion access while the daemon is running, and ask the operating system to remove it when the daemon process is no longer running. Regardless of if it shut down cleanly or not.

If you would, please create a separate issue for that one.

@Stebalien
Copy link
Member

Unfortunately, we can't get operating systems like linux/macos to do the same thing. Take a look at #5784. I'd like to modify go-ipfs to try the API and then automatically fall back on the local repo.

@LetItGlow
Copy link
Author

I have a hunch why this "api" file exists. With manipulation, I can use an IPFS instance from Node A+it's own repo to redirect to the API and repo of a Node B daemon.. Both nodes are locally though.
(I run several nodes in private mode on the same computer - no VM, just my little pseudo-dev setup to "play" with IPFS)
I literally just destroyed my 3rd private node by wiping it's repo and left ONLY the "API" and "swarm.key" files, pointing the API to the second node while the daemon was running... it worked.
ipfs id returned the node ID of the Node B, as everything else possible with API access, athough THAT was not it's own repo.
I am not saying it's bad, it has a VERY rare special case use without an config file.

Probably it could use the config file to read the API setting directly and try to connect to that, if there is no "hello", default to standalone mode. With my bad skills, i'd probably check for a repo.lock, if there is none, there is no daemon. quick, simple but dirty. and prone to throw an error, when there is a lingering repo.lock

However, when I was experimenting with GETH, the CLI could autodetermine the (running) server using RPC or Mutex or Pipe whatever, it checked for that existing and connected to that automatically. GETH is written in go, too as I just noticed.
Sorry for the offtopic rant.

@Stebalien
Copy link
Member

I am not saying it's bad, it has a VERY rare special case use without an config file.

It's not actually that rare. I use this feature and I know several other devs use it.

Probably it could use the config file to read the API setting directly and try to connect to that, if there is no "hello", default to standalone mode. With my bad skills, i'd probably check for a repo.lock, if there is none, there is no daemon. quick, simple but dirty. and prone to throw an error, when there is a lingering repo.lock

Unfortunately, reading the config isn't sufficient. The config may list the unspecified/all address (0.0.0.0). The API file contains the address the API is actually listening on.

However, when I was experimenting with GETH, the CLI could autodetermine the (running) server using RPC or Mutex or Pipe whatever, it checked for that existing and connected to that automatically. GETH is written in go, too as I just noticed.

We'd eventually like to support connecting to a daemon via a unix socket as well. However, we'd still likely ahve to support both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) status/in-progress In progress topic/windows Windows specific
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants