search

edicl / drakma

HTTP client written in Common Lisp
http://edicl.github.io/drakma/
249 stars 58 forks source link

incorrect POSIX return code handling on connect() #59

Closed mateuszb closed 9 years ago

mateuszb commented 9 years ago

connect() can return error code -1 and set errno to EINTR, which doesn't mean the connection didn't succeed.

"If connect() is interrupted by a signal that is caught while blocked waiting to establish a connection, connect() shall fail and set errno to [EINTR], but the connection request shall not be aborted, and the connection shall be established asynchronously." http://pubs.opengroup.org/onlinepubs/009695399/functions/connect.html

Currently, usocket and drakma fail with multiple threads trying to connect to different http endpoints and signal arrives and connect() returns EINTR which is far from desired. One way to work around is perform asynchronous connect from the beginning, but the usocket library doesn't support that .

stassats commented 9 years ago

This is for usocket, not drakma. And usocket has different backends for different implementations, so when you open a ticket for usocket, you should specify on which implementations connect is misbehaving.

mateuszb commented 9 years ago

I believe this is for drakma, not for usocket. usocket behaves correctly when this happens and according to the spec. drakma doesn't handle the error code returned from usocket properly and thus tells the http stream has not been opened when in fact there is an established connection lingering and this leads to resource leakage.

stassats commented 9 years ago

It shouldn't signal an error, it should restart syscalls automatically.

mateuszb commented 9 years ago

POSIX spec says connect() shouldn't and can't be restarted after EINTR, (it will return EADDRINUSE or EALREADY immediately) and it is not a "connection failed" indicator

hanshuebner commented 9 years ago

USOCKET is not POSIX. I agree with Stars, not a DRAKMA bug. Am 13.06.2015 10:13 nachm. schrieb "Mateusz Berezecki" < notifications@github.com>:

POSIX spec says connect() shouldn't and can't be restarted after EINTR, (it will return EADDRINUSE or EALREADY immediately) and it is not a "connection failed" indicator

— Reply to this email directly or view it on GitHub https://github.com/edicl/drakma/issues/59#issuecomment-111744488.

mateuszb commented 9 years ago

makes drakma absolutely unusable in multithreaded SBCL environment on OS X kernels :-/

does drakma support switching socket libraries or something like this ?

hanshuebner commented 9 years ago

I do not follow your reasoning. Am 13.06.2015 10:18 nachm. schrieb "Mateusz Berezecki" < notifications@github.com>:

makes drakma absolutely unusable in multithreaded SBCL environment on OS X kernels :-/

does drakma support switching socket libraries or something like this ?

— Reply to this email directly or view it on GitHub https://github.com/edicl/drakma/issues/59#issuecomment-111746208.

stassats commented 9 years ago

This doesn't change the situation, drakma is "unusable" as it is, and needs fixing. But instead of fixing drakma usocket needs to be fixed. I don't see why you would consider fixing drakma being the easier option.

hanshuebner commented 9 years ago

I also don't think that working around the issue in DRAKMA would be the better solution. In fact, it supports "switching backendes" because it uses LispWork's native socket in that environment. It makes certain assumptions on the underlying socket library, connection attempts returning an error only being one of them. Just because POSIX decides to signal certain events using the error path does not mean that this needs to be exposed to USOCKET clients.

2015-06-13 22:24 GMT+02:00 Stas Boukarev notifications@github.com:

This doesn't change the situation, drakma is "unusable" as it is, and needs fixing. But instead of fixing drakma usocket needs to be fixed. I don't see why you would consider fixing drakma being the easier option.

— Reply to this email directly or view it on GitHub https://github.com/edicl/drakma/issues/59#issuecomment-111747368.

mateuszb commented 9 years ago

That's fair point. I guess I'll see if I can add iolib sockets support

mateuszb commented 9 years ago

Ok, to follow up. I have submitted a diff against SBCL and waiting on this being resolved. I have also made a patch that adds SBCLs native sockets in drakma if you guys are interested in that.

hanshuebner commented 9 years ago

What would be the advantage of having a DRAKMA version that does not require USOCKET?

2015-06-14 2:37 GMT+02:00 Mateusz Berezecki notifications@github.com:

Ok, to follow up. I have submitted a diff against SBCL and waiting on this being resolved. I have also made a patch that adds SBCLs native sockets in drakma if you guys are interested in that.

— Reply to this email directly or view it on GitHub https://github.com/edicl/drakma/issues/59#issuecomment-111765836.

mateuszb commented 9 years ago

I don't know but it seems to already special case #+:lispworks

Inviato da iPhone

Il giorno 13/giu/2015, alle ore 21:42, Hans Hübner notifications@github.com ha scritto:

What would be the advantage of having a DRAKMA version that does not require USOCKET?

2015-06-14 2:37 GMT+02:00 Mateusz Berezecki notifications@github.com:

Ok, to follow up. I have submitted a diff against SBCL and waiting on this being resolved. I have also made a patch that adds SBCLs native sockets in drakma if you guys are interested in that.

— Reply to this email directly or view it on GitHub https://github.com/edicl/drakma/issues/59#issuecomment-111765836.

— Reply to this email directly or view it on GitHub.

hanshuebner commented 9 years ago

2015-06-14 6:52 GMT+02:00 Mateusz Berezecki notifications@github.com:

I don't know but it seems to already special case #+:lispworks

That is a historic thing which is still there because Edi likes it. I would not want to add another backend if there is no compelling reason to do so.

mateuszb commented 9 years ago

Alright. Seems like this is not the preferred way to go. Where do you guys suggest I go and fix this ? This is an actual problem and everywhere I go people involved say this is the other person's fault.

Suggestions on how to proceed ? This is really broken on Mac OS X. I don't want to always keep private patches and forward port them so I thought contributing back somewhere would be nice and the right thing to do.

Thoughts ?

Inviato da iPhone

Il giorno 13/giu/2015, alle ore 22:05, Hans Hübner notifications@github.com ha scritto:

2015-06-14 6:52 GMT+02:00 Mateusz Berezecki notifications@github.com:

I don't know but it seems to already special case #+:lispworks

That is a historic thing which is still there because Edi likes it. I would not want to add another backend if there is no compelling reason to do so. — Reply to this email directly or view it on GitHub.

hanshuebner commented 9 years ago

We've already said that the bug needs to be fixed in USOCKET. Is there a problem with that?

2015-06-14 7:16 GMT+02:00 Mateusz Berezecki notifications@github.com:

Alright. Seems like this is not the preferred way to go. Where do you guys suggest I go and fix this ? This is an actual problem and everywhere I go people involved say this is the other person's fault.

Suggestions on how to proceed ? This is really broken on Mac OS X. I don't want to always keep private patches and forward port them so I thought contributing back somewhere would be nice and the right thing to do.

Thoughts ?

Inviato da iPhone

Il giorno 13/giu/2015, alle ore 22:05, Hans Hübner < notifications@github.com> ha scritto:

2015-06-14 6:52 GMT+02:00 Mateusz Berezecki notifications@github.com:

I don't know but it seems to already special case #+:lispworks

That is a historic thing which is still there because Edi likes it. I would not want to add another backend if there is no compelling reason to do so. — Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub https://github.com/edicl/drakma/issues/59#issuecomment-111784222.

mateuszb commented 9 years ago

Nope. I just thought thought adding more flexibility would be nice (hence my iolib or sbcl native socket option suggestions). I think usocket is horribly incomplete (not even supporting o-nonblock) which makes it totally uninteresting for high performance server design and production code. I fixed the problem for myself in sbcl and started a discussion about that on that level. If the preferred route is to fix it in usocket I will leave it for someone else.

Inviato da iPhone

Il giorno 13/giu/2015, alle ore 22:18, Hans Hübner notifications@github.com ha scritto:

We've already said that the bug needs to be fixed in USOCKET. Is there a problem with that?

2015-06-14 7:16 GMT+02:00 Mateusz Berezecki notifications@github.com:

Alright. Seems like this is not the preferred way to go. Where do you guys suggest I go and fix this ? This is an actual problem and everywhere I go people involved say this is the other person's fault.

Suggestions on how to proceed ? This is really broken on Mac OS X. I don't want to always keep private patches and forward port them so I thought contributing back somewhere would be nice and the right thing to do.

Thoughts ?

Inviato da iPhone

Il giorno 13/giu/2015, alle ore 22:05, Hans Hübner < notifications@github.com> ha scritto:

2015-06-14 6:52 GMT+02:00 Mateusz Berezecki notifications@github.com:

I don't know but it seems to already special case #+:lispworks

That is a historic thing which is still there because Edi likes it. I would not want to add another backend if there is no compelling reason to do so. — Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub https://github.com/edicl/drakma/issues/59#issuecomment-111784222.

— Reply to this email directly or view it on GitHub.

hanshuebner commented 9 years ago

2015-06-14 7:24 GMT+02:00 Mateusz Berezecki notifications@github.com:

Nope. I just thought thought adding more flexibility would be nice (hence my iolib or sbcl native socket option suggestions). I think usocket is horribly incomplete (not even supporting o-nonblock) which makes it totally uninteresting for high performance server design and production code. I fixed the problem for myself in sbcl and started a discussion about that on that level. If the preferred route is to fix it in usocket I will leave it for someone else.

While I agree that USOCKET is not suitable for asynchronous networking, it is quite sufficient for DRAKMA and I do not see how new backends could improve DRAKMA at this point. Just because DRAKMA uses USOCKET does not imply that if you use DRAKMA, you need to use USOCKET for your own code as well, or does it?

If you are not interested in fixing USOCKET, you could at least open a bug report at https://github.com/usocket/usocket/issues because what you've found is a bug in USOCKET. I am pretty sure that it will be fixed by someone else if you cannot supply a patch.

mateuszb commented 9 years ago

Il giorno 13/giu/2015, alle ore 22:31, Hans Hübner notifications@github.com ha scritto:

2015-06-14 7:24 GMT+02:00 Mateusz Berezecki notifications@github.com:

Nope. I just thought thought adding more flexibility would be nice (hence my iolib or sbcl native socket option suggestions). I think usocket is horribly incomplete (not even supporting o-nonblock) which makes it totally uninteresting for high performance server design and production code. I fixed the problem for myself in sbcl and started a discussion about that on that level. If the preferred route is to fix it in usocket I will leave it for someone else.

While I agree that USOCKET is not suitable for asynchronous networking, it is quite sufficient for DRAKMA and I do not see how new backends could improve DRAKMA at this point. Just because DRAKMA uses USOCKET does not imply that if you use DRAKMA, you need to use USOCKET for your own code as well, or does it?

Agreed for the intended scenario. I keep a set of patches that makes drakma asynchronous though where I replaced usocket entirely. That's how I discovered this connect system call problem on OSX.

If you are not interested in fixing USOCKET, you could at least open a bug report at https://github.com/usocket/usocket/issues because what you've found is a bug in USOCKET. I am pretty sure that it will be fixed by someone else if you cannot supply a patch.

Yeah, don't get me wrong I will open a ticket for sure so people are aware of this and that it can be tracked.

Mateusz

— Reply to this email directly or view it on GitHub.

hanshuebner commented 9 years ago

2015-06-14 7:34 GMT+02:00 Mateusz Berezecki notifications@github.com:

Agreed for the intended scenario. I keep a set of patches that makes drakma asynchronous though where I replaced usocket entirely. That's how I discovered this connect system call problem on OSX.

How much of DRAKMA's API is supported if it runs asynchronously with your patches?

tmccombs commented 9 years ago

@mateuszb have you looked at drakm-async (https://github.com/orthecreedence/drakma-async)? It is missing recent changes to drakma.