Skip to content
New issue

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

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

Already on GitHub? # to your account

Running in parallel causes procedure corruption #101

Closed
maxiwheat opened this issue May 12, 2016 · 5 comments
Closed

Running in parallel causes procedure corruption #101

maxiwheat opened this issue May 12, 2016 · 5 comments
Assignees
Labels
Milestone

Comments

@maxiwheat
Copy link

maxiwheat commented May 12, 2016

We run a program in parallel (multiple instances of PHP running a script) that call php phantomjs. I think we end up in situations where the content of the generated script is corrupted because two process try to write to the same file. Here is the end of a script found in /tmp :

system.stderr.write(debug.join('\\n') + '\\n');
system.stdout.write(JSON.stringify(response, undefined, 4));

phantom.exit();

};

m.exit();

};

See how it ends, m.exit(); is not an actual command... it looks more like the end of phantom.exit(); appended to the file.

I think the error comes from how the unique names are generated (using uniqid)

@jonnnnyw
Copy link
Owner

Yeah, uniqid is time based so you are probably right. I will ditch it and use something else.

Do you have a simple way to replicate the issue so that I can test it?

@jonnnnyw jonnnnyw self-assigned this May 12, 2016
@jonnnnyw jonnnnyw added the bug label May 12, 2016
@maxiwheat
Copy link
Author

Sorry no, happens rarely ... but has happen 2 times in the past 2 or 3 weeks, it's very hard to reproduce, but I ended looking at the generated script since we run 2 process in parallel with Gearman. Running htop in console I noticed that phantomjs /tmp/5728c811e5ad4 appeared twice which should not happen, that gave me the idea of a possible "overlap" of temp file names.

jonnnnyw added a commit that referenced this issue May 20, 2016
…icts when running processes in parallel. Have replaced for random string generation seeded by mt_rand - #101
@jonnnnyw jonnnnyw added this to the Version 4.5 milestone May 23, 2016
@jonnnnyw
Copy link
Owner

I have replaced uniqid with a (hopefully) more random solution. This has been released in v4.5 so let me know if you have any further issues with it.

@maxiwheat
Copy link
Author

maxiwheat commented Jun 22, 2016

Looks like to work, thanks a lot

@batyrmastyr
Copy link

@jonnnnyw You'd rather send getmypid() as $prefix parameter to uniqid(). On top of that, there is $more_entropy parameter just in case if someone will run script using pthreads.
Mt_rand() just makes issue undebugable, moreover calling md5() over mt_rand() result just wastes cpu without any benefit.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants