sharplispers / clx

a fork of crhodes' fork of danb's fork of the CLX library, an X11 client for Common Lisp
Other
114 stars 46 forks source link

dependent: Fix the SBCL side of #200 (EOF error on KillClient) #203

Closed abridgewater closed 9 months ago

abridgewater commented 10 months ago
dkochmanski commented 9 months ago

I'd rather see this fix to improve behavior of all supported implementations, having sbcl as a single (correct) outlier, in case of issue reports, will only make debugging harder.

abridgewater commented 9 months ago

I'd rather see this fix to improve behavior of all supported implementations, having sbcl as a single (correct) outlier, in case of issue reports, will only make debugging harder.

On the one hand, I can see where you're coming from, and even agree with that stance to a point. On the other hand, if nobody does the additional work, is having no correct platforms really a win?

That said, the new commit should cover ECL (and note the more obtuse error message than for SBCL), and testing to see if the same change works on CLISP and CLASP should be straightforward for someone who has them installed. I am not and will not be that someone.

dkochmanski commented 9 months ago

I see your point (although I don't find it compelling) -- I'll finish this pull request for other FOSS implementations then; please tell me also:

abridgewater commented 9 months ago

I see your point (although I don't find it compelling) -- I'll finish this pull request for other FOSS implementations then; please tell me also:

Thank you!

* what obtuse ECL error should I take note of?

I quoted it in the commit message, the "Asynchronous UNKNOWN-ERROR". This happens because READ-SEQUENCE returns without having filled the buffer, but CLX is being told that the buffer has been filled, and tries to parse it as an event from the server.

* why do we prefer read-n-bytes over read-sequence?

My best guess here is that it's a historical artifact: CMUCL has READ-N-BYTES as an extension because READ-SEQUENCE wasn't in CLtL1, and whoever did the original port of CLX to CMUCL might not have been familiar with READ-SEQUENCE (or for all I know it could have been from before CMUCL supported READ-SEQUENCE). SBCL being a fork of CMUCL then continued using the same extension (though SBCL itself deems it to be an internal interface rather than for general use).

CLX itself clearly predates the ANSI standard, and thus presumably READ-SEQUENCE, otherwise it would have been used in the fallback implementation of BUFFER-READ-DEFAULT (and the 1987 copyright date in dependent.lisp also supports the claim of being pre-ANSI).

dkochmanski commented 9 months ago

Steps to reproduce:

> (ql:quickload 'clx/demo)
> (xlib-demo/demos:demo)

In a separate console launch xkill and select the window.

Correct behavior (in REPL):

CL-USER> (xlib-demo/demos:demo)
NIL
#<XLIB:CLOSED-DISPLAY {10087BB1F3}>

Incorrect behavior:

CL-USER> (xlib-demo/demos:demo)
NIL
#<SB-INT:BROKEN-PIPE "~@<~?~@[: ~2I~_~A~]~:>" {10091E3EF3}>

(substitute sbcl error with implementation-specific one).

dkochmanski commented 9 months ago

I'm closing this pull request in favor of #205. Please add that thread to notifications if you are interested in when it is merged. Thanks.