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

Can't read/write to the same cosocket from different threads #241

Closed
AdallomRoy opened this issue May 27, 2013 · 12 comments
Closed

Can't read/write to the same cosocket from different threads #241

AdallomRoy opened this issue May 27, 2013 · 12 comments

Comments

@AdallomRoy
Copy link

It's impossible with the current implementation to read from thread 1 and write from thread 2 to the same socket.
This causes issues in tunnel cases - if I wanted to create a tunnel between 2 connections I'd run 2 threads, 1 reading from socket A and writing to socket B, the other reading from socket B and writing to socket A - which is currently impossible ("socket busy") - even though this supported by nginx itself.
This is caused by the fact that there isn't a separation of read & write state for each socket -
ngx_http_lua_co_ctx_t *co_ctx;
and
unsigned waiting:1;
are for both read & write operations - They should probably be separated.

@agentzh
Copy link
Member

agentzh commented May 27, 2013

Actually there is a deeper limitation here in the current implementation, that is, a cosocket object cannot be shared between different requests at all. When a cosocket object is accessed from within another request that did not create, the behavior is actually undefined.

Also, please be aware that cosockets also cannot be shared among different Nginx worker processes and you cannot really control which worker process to serve a particular request unless you have only one Nginx worker process.

@AdallomRoy
Copy link
Author

Hi!

Thanks for the quick reply.
I understand that things that are not supported by nginx (e.g. sharing an nginx socket between different processes) are also not supported by lua-nginx - it's totally reasonable.
However, I'm talking about a simple scenario that nginx itself does support yet lua-nginx does not due to implementation issues - which is a tunnelling scenario:

User sends a request to nginx - and keeps the connection alive - all the data is above the same request (we'll call the user with nginx socket A)
nginx uses lua content handler (with socket which we'll call B) to tunnel the request to a different backend using TCP.

Now we hold 2 sockets - A & B,
In order to tunnel the connections we need 2 lua threads.
Thread 1 - Reading from socket A and writing to socket B
Thread 2 - Reading from socket B and writing to socket A
until one of the connections is closed and then the request is done.

This is a very simple proxy scenario :)

This can be achieved by using nginx sockets & C content handler - but not by using lua-nginx, since the read and write must be performed in the same thread for the same socket in the current implementation.

I suggest splitting the co_ctx to read_co_ctx and write_co_ctx, and also split the waiting to read_waiting and write_waiting. this will allow much more flexibility with the cosocket API.

Cheers.

@bakins
Copy link
Member

bakins commented May 28, 2013

Technically you aren't waiting to read and write, however it does appear that you are bcs you cannot just "schedule IO." Have you tried using "threads" http://wiki.nginx.org/HttpLuaModule#ngx.thread.spawn . or just reading in "chunks" - read 8k (for example) from A and write to B, etc.
As I said, technically, Lua is not waiting on reads and writes as it just yields to the event loop. A more concrete example may be needed as I use Lua in plenty of "simple proxy" scenarios.

@AdallomRoy
Copy link
Author

Yes, I am using ngx.thread.spawn - but the threads receive a "socket busy" error - because you can't read in one thread from a socket and write in a different thread.
I know that Lua is not waiting on reads and writes, but using the current implementation it's impossible to add both a read and a write event to the nginx reactor if the threads writing the events are different (lua-nginx limitation)

The suggestion of reading in chunks is fine (a patch here is also needed to prevent lua-nginx from closing sockets on time-out, but irrelevant to the subject) - and it works, but it then simple generates a busy-loop instead of using nginx's event based model - which is a very large performance hit...

@agentzh
Copy link
Member

agentzh commented May 28, 2013

@AdallomRoy okay, I see your point now. You're just proxying the downstream cosockets and upstream cosockets. This is technically possible by separating the read and write states in a single cosocket object. Patches welcome! :)

@agentzh
Copy link
Member

agentzh commented May 28, 2013

@AdallomRoy BTW, there is a pending patch on the openresty-en mailing list that makes read timeout errors nonfatal as you've mentioned: https://groups.google.com/group/openresty-en/browse_thread/thread/f65a5fcd77f4e5cc I'm going to review it in the near future.

@AdallomRoy
Copy link
Author

Hi!

@agentzh , Thanks.
This patch can get a little tricky, but I think I might have time this/next week.
It also requires adding some tests since it actually changes some logic :)

Cheers.

@bungle
Copy link
Member

bungle commented Sep 20, 2013

I think I have another use case described here:
openresty/lua-resty-websocket#1 (comment)

@AdallomRoy
Copy link
Author

Sorry about closing the issue, this was by mistake :)

What I wanted to say is that I already created a patch that does this functionality. It's already tested and runs in production.
However, I need to adjust it to the latest version of ngx_lua (my patch is for a version a few months old) and also do some code clean-ups.
I hope I'll get to it soon.

@bungle
Copy link
Member

bungle commented Sep 30, 2013

@AdallomRoy Hi, I just found out that @agentzh included lua-resty-websocket in the latest OpenResty bundle. Do you have any news on this front (just being curious, :-))?

@AdallomRoy
Copy link
Author

@aviramc added our patch for this issue, as well as other several open issues.
Enjoy!

@agentzh
Copy link
Member

agentzh commented Aug 8, 2014

Now that we already have full-duplex cosocket support in ngx_lua 0.9.9+, I'm closing this.

@agentzh agentzh closed this as completed Aug 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants