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

read just one line when checking md5 #732

Merged
merged 1 commit into from Sep 29, 2017

Conversation

Projects
None yet
3 participants
@mikz
Contributor

mikz commented Sep 26, 2017

in some cases reading the whole file fails (on macOS High Sierra, LuaJIT) (by returning nil)
this is timing dependent and waiting between spawning and reading solves the issue as well

read just one line when checking md5
in some cases reading the whole file fails (on macOS High Sierra, LuaJIT)
this is timing dependent and waiting between spawning and reading solves the issue as well
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 26, 2017

Codecov Report

Merging #732 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #732   +/-   ##
=======================================
  Coverage   85.48%   85.48%           
=======================================
  Files          63       63           
  Lines        6731     6731           
=======================================
  Hits         5754     5754           
  Misses        977      977
Impacted Files Coverage Δ
src/luarocks/fs/tools.lua 84.81% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 147003f...2de11bd. Read the comment docs.

codecov-io commented Sep 26, 2017

Codecov Report

Merging #732 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #732   +/-   ##
=======================================
  Coverage   85.48%   85.48%           
=======================================
  Files          63       63           
  Lines        6731     6731           
=======================================
  Hits         5754     5754           
  Misses        977      977
Impacted Files Coverage Δ
src/luarocks/fs/tools.lua 84.81% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 147003f...2de11bd. Read the comment docs.

@hishamhm

This comment has been minimized.

Show comment
Hide comment
@hishamhm

hishamhm Sep 26, 2017

Member

Hi? What LuaJIT version is this on? Sounds like a bug on their (or the OS's?) end.

Member

hishamhm commented Sep 26, 2017

Hi? What LuaJIT version is this on? Sounds like a bug on their (or the OS's?) end.

@mikz

This comment has been minimized.

Show comment
Hide comment
Contributor

mikz commented Sep 26, 2017

@mikz

This comment has been minimized.

Show comment
Hide comment
@mikz

mikz Sep 26, 2017

Contributor

@hishamhm I know it sounds like interpreter/OS bug, but the chance of fixing it is higher here than on interpreter/OS level.

Even putting some sleep in between spawning the command and reading fixes the issue.

The code is correct - it needs just one line, right? Then this patch is a good workaround until this gets fixed in different levels.

Contributor

mikz commented Sep 26, 2017

@hishamhm I know it sounds like interpreter/OS bug, but the chance of fixing it is higher here than on interpreter/OS level.

Even putting some sleep in between spawning the command and reading fixes the issue.

The code is correct - it needs just one line, right? Then this patch is a good workaround until this gets fixed in different levels.

@hishamhm

This comment has been minimized.

Show comment
Hide comment
@hishamhm

hishamhm Sep 26, 2017

Member

Could you try a stable (or at least beta) LuaJIT release and confirm that the bug is in their end?

That version you linked isn't even an official LuaJIT version...

Member

hishamhm commented Sep 26, 2017

Could you try a stable (or at least beta) LuaJIT release and confirm that the bug is in their end?

That version you linked isn't even an official LuaJIT version...

@hishamhm

This comment has been minimized.

Show comment
Hide comment
@hishamhm

hishamhm Sep 26, 2017

Member

Also:

The code is correct - it needs just one line, right? Then this patch is a good workaround until this gets fixed in different levels.

Agree, but it also important to get proper diagnostics to know whose fault it is, so we can notify the proper parties.

Member

hishamhm commented Sep 26, 2017

Also:

The code is correct - it needs just one line, right? Then this patch is a good workaround until this gets fixed in different levels.

Agree, but it also important to get proper diagnostics to know whose fault it is, so we can notify the proper parties.

@mikz

This comment has been minimized.

Show comment
Hide comment
@mikz

mikz Sep 26, 2017

Contributor

I have a working example: 3scale/apicast@e6d5e27

Hm. When trying to reproduce it now (after upgrading to macOS HIgh Sierra GM from Beta 3) I get different error:

when executing
cd '/Users/mikz/Developer/apicast/lua_modules/lib/luarocks/rocks/lua-resty-http/0.10-0' && find * 2> /dev/null

ERROR: /usr/local/share/lua/5.1/luarocks/fs/unix/tools.lua:115: Interrupted system call

Any ideas how to debug that?

Contributor

mikz commented Sep 26, 2017

I have a working example: 3scale/apicast@e6d5e27

Hm. When trying to reproduce it now (after upgrading to macOS HIgh Sierra GM from Beta 3) I get different error:

when executing
cd '/Users/mikz/Developer/apicast/lua_modules/lib/luarocks/rocks/lua-resty-http/0.10-0' && find * 2> /dev/null

ERROR: /usr/local/share/lua/5.1/luarocks/fs/unix/tools.lua:115: Interrupted system call

Any ideas how to debug that?

@hishamhm

This comment has been minimized.

Show comment
Hide comment
@hishamhm

hishamhm Sep 26, 2017

Member

The first thing I'd try would be to run the script with a luajit interpreter directly and not resty. (That's available at <prefix_of_openresty>/luajit/bin/luajit).

If that still fails, I'd try a stable/beta LuaJIT release and see if it makes a difference.

Member

hishamhm commented Sep 26, 2017

The first thing I'd try would be to run the script with a luajit interpreter directly and not resty. (That's available at <prefix_of_openresty>/luajit/bin/luajit).

If that still fails, I'd try a stable/beta LuaJIT release and see if it makes a difference.

@mikz

This comment has been minimized.

Show comment
Hide comment
@mikz

mikz Sep 26, 2017

Contributor

@hishamhm you are right. It fails with resty but works with plain luajit interpreter. And I get various errors with the resty interpreter. Either the interrupted system call or
ERROR: bin/dependencies:23: Failed producing checksum: Failed to compute MD5 hash for file /Users/mikz/Developer/apicast/lua_modules/lib/luarocks/rocks/lua-resty-http/0.10-0/doc/LICENSE

Going to report it to resty-cli. (openresty/resty-cli#35)

Contributor

mikz commented Sep 26, 2017

@hishamhm you are right. It fails with resty but works with plain luajit interpreter. And I get various errors with the resty interpreter. Either the interrupted system call or
ERROR: bin/dependencies:23: Failed producing checksum: Failed to compute MD5 hash for file /Users/mikz/Developer/apicast/lua_modules/lib/luarocks/rocks/lua-resty-http/0.10-0/doc/LICENSE

Going to report it to resty-cli. (openresty/resty-cli#35)

@hishamhm

This comment has been minimized.

Show comment
Hide comment
@hishamhm

hishamhm Sep 26, 2017

Member

Going to report it to resty-cli.

@mikz They're probably going to say you shouldn't be using LuaRocks from within OpenResty, because it does blocking I/O. The resty wrapper is actually an NGINX launcher. Doesn't seem to be what you want to use there anyway.

Member

hishamhm commented Sep 26, 2017

Going to report it to resty-cli.

@mikz They're probably going to say you shouldn't be using LuaRocks from within OpenResty, because it does blocking I/O. The resty wrapper is actually an NGINX launcher. Doesn't seem to be what you want to use there anyway.

@mikz

This comment has been minimized.

Show comment
Hide comment
@mikz

mikz Sep 26, 2017

Contributor

@hishamhm I hope not because resty is supposed to handle blocking io just fine (but blocking). Even documentation mentions blocking io in the init phase.

In my case resty is the only available interpreter on the PATH, so thats what I have.

Closing this as this fix would not fix the other error (Interrupted system call) anyway.

Contributor

mikz commented Sep 26, 2017

@hishamhm I hope not because resty is supposed to handle blocking io just fine (but blocking). Even documentation mentions blocking io in the init phase.

In my case resty is the only available interpreter on the PATH, so thats what I have.

Closing this as this fix would not fix the other error (Interrupted system call) anyway.

@mikz mikz closed this Sep 26, 2017

@mikz mikz reopened this Sep 28, 2017

@mikz

This comment has been minimized.

Show comment
Hide comment
@mikz

mikz Sep 28, 2017

Contributor

OpenResty suggests using *l as a workaround. openresty/resty-cli#35 (comment)

Would you be ok if I'd fix all the occurrences of this?

Contributor

mikz commented Sep 28, 2017

OpenResty suggests using *l as a workaround. openresty/resty-cli#35 (comment)

Would you be ok if I'd fix all the occurrences of this?

@mikz

This comment has been minimized.

Show comment
Hide comment
@mikz

mikz Sep 29, 2017

Contributor

Also this patch fixes the lines issue, but definitely should be used only on demand:

local lines = getmetatable(io.output()).lines

getmetatable(io.output()).lines = function(self, ...)
    local iter = lines(self, ...)

    return function()
        local ok, ret = pcall(iter)

        if ok then return ret
        else return nil end
    end
end
Contributor

mikz commented Sep 29, 2017

Also this patch fixes the lines issue, but definitely should be used only on demand:

local lines = getmetatable(io.output()).lines

getmetatable(io.output()).lines = function(self, ...)
    local iter = lines(self, ...)

    return function()
        local ok, ret = pcall(iter)

        if ok then return ret
        else return nil end
    end
end
@hishamhm

This comment has been minimized.

Show comment
Hide comment
@hishamhm

hishamhm Sep 29, 2017

Member

oh, btw, instead of writing that script you could just write a rockspec will all your dependencies (using dependencies = { "foo == 1.0", "bar == 2.3" } to pin versions, and run

luarocks install --only-deps --tree=lua_modules my-dev-environment-1.0-1.rockspec
Member

hishamhm commented Sep 29, 2017

oh, btw, instead of writing that script you could just write a rockspec will all your dependencies (using dependencies = { "foo == 1.0", "bar == 2.3" } to pin versions, and run

luarocks install --only-deps --tree=lua_modules my-dev-environment-1.0-1.rockspec

@hishamhm hishamhm merged commit e64f83b into luarocks:master Sep 29, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mikz mikz deleted the mikz:patch-1 branch Sep 29, 2017

@mikz

This comment has been minimized.

Show comment
Hide comment
@mikz

mikz Sep 29, 2017

Contributor

@hishamhm I know. But I want transitive dependency locking being automatic like cargo or bundler does. So I'm writing a wrapper around luarocks (and opm in the future) to provide explicit transitive locking via a lockfile.

Contributor

mikz commented Sep 29, 2017

@hishamhm I know. But I want transitive dependency locking being automatic like cargo or bundler does. So I'm writing a wrapper around luarocks (and opm in the future) to provide explicit transitive locking via a lockfile.

@hishamhm

This comment has been minimized.

Show comment
Hide comment
@hishamhm

hishamhm Sep 29, 2017

Member

Instead of a wrapper, let's add this feature into LuaRocks instead! I'd be happy to merge in this feature and helping to getting a PR in. :)

Member

hishamhm commented Sep 29, 2017

Instead of a wrapper, let's add this feature into LuaRocks instead! I'd be happy to merge in this feature and helping to getting a PR in. :)

@mikz

This comment has been minimized.

Show comment
Hide comment
@mikz

mikz Sep 29, 2017

Contributor

@hishamhm That would be nice. But for now I'd prefer prototyping it outside. Also I'm not sure how the OPM support would integrate with LuaRocks. The repository will be soon up on https://github.com/3scale/lua-rover with first prototype.

Contributor

mikz commented Sep 29, 2017

@hishamhm That would be nice. But for now I'd prefer prototyping it outside. Also I'm not sure how the OPM support would integrate with LuaRocks. The repository will be soon up on https://github.com/3scale/lua-rover with first prototype.

@hishamhm

This comment has been minimized.

Show comment
Hide comment
@hishamhm

hishamhm Sep 29, 2017

Member

@mikz I see you're using the LuaRocks internal modules directly. This is good because it will make it easy to move the functionality inside LuaRocks once you're done prototyping, but it's also a bit concerning because those are not public APIs guaranteed to be stable (even though in practice they have been mostly stable and a proper stable LuaRocks-as-a-library module is in the works).

Member

hishamhm commented Sep 29, 2017

@mikz I see you're using the LuaRocks internal modules directly. This is good because it will make it easy to move the functionality inside LuaRocks once you're done prototyping, but it's also a bit concerning because those are not public APIs guaranteed to be stable (even though in practice they have been mostly stable and a proper stable LuaRocks-as-a-library module is in the works).

thibaultcha added a commit to Kong/kong that referenced this pull request Dec 8, 2017

hotfix(tests) ensure bin/busted works on some platforms
A regression introduced by 67473d2 and #3075: the underlying `io.popen`
read syscall can easily be interrupted by the `SIGCHLD` signal returned
from the invoked shell (handled by NGINX, underlying the resty-cli)
interpreter.

This issue is discussed in several places:

* openresty/resty-cli#35
* luarocks/luarocks#732

An easy workaround for it is the fix proposed in one of the above
issues, in which we simply call our iterator in protected mode and
ignore the resulting Lua error from the uncaught `EINTR` error.

thibaultcha added a commit to Kong/kong that referenced this pull request Dec 9, 2017

hotfix(tests) ensure bin/busted works on some platforms
A regression introduced by 67473d2 and #3075: the underlying `io.popen()`
read syscall can easily be interrupted by the `SIGCHLD` signal returned
from the invoked shell (handled by NGINX, underlying the resty-cli)
interpreter.

This issue is discussed in several places:

* openresty/resty-cli#35
* luarocks/luarocks#732

An easy workaround for it is the fix proposed in one of the above
issues, in which we simply call our iterator in protected mode and
ignore the resulting Lua error from the uncaught `EINTR` error.

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