Update ZdcSimpleSocketStream for recent changes to Socket

[daniels220]

Socket>>waitForSendDoneFor: now raises an exception directly rather than returning a boolean

Pending merge of pharo#15984

[jbrichau]

I ended up at this PR trying to debug and fix issue https://github.com/pharo-project/pharo/issues/16373 which blocks any use of Seaside/Zinc on Pharo 12 (on a Mac at least).

@svenvc @daniels220 Would this change be acceptable if it would be adapted such that it does not break for older Pharo versions? As far as I can see that would be the issue with the PR right now...

[svenvc]

No longer signalling the exception seems like a non-trivial change, a change in semantics. Now, if everything keeps working, we might try it, simplification is always a good thing. But networking it tricky, it is really hard to simulate slower or less reliable connections.

[daniels220]

@svenvc Worth noting that it's signaling the exception that is the nontrivial change in semantics—IOW the exception is new, the boolean return was the behavior in Pharo 11. But nothing is actually different about how it interacts with the network layer, it's just moving the exception throw into the wait... method—prior to that, ~all self-sends on Socket (and this one in Zodiac) immediately did ifFalse: [ConnectionTimedOut signal: ...], which was much of the motivation for refactoring so the exception is raised internally in the first place (along with distinguishing timeout from closed).

As far as adapting this change to work in both Pharo 11 and 12+, how has this been dealt with in the past? Surely this is not the first time Zinc as a whole has encountered a breaking change in Pharo? I know of at least three ways to handle it:

• Separate extension packages for different Pharo versions, with the baseline choosing which to load appropriately. Grease and Seaside stand out here.
• Separate Git branches for different Pharo versions—generally maintaining only one version of backcompat as cross-porting workload increases rapidly. I know I've seen this at least once but forget where.
• Carefully writing code to shim for old versions yet not break in the current version—which is not always possible, so, falling back to either of the above approaches, or explicit version checks in the code.

It turns out the third approach is possible here—I would encapsulate within socketWaitForSendDone:

socketWaitForSendDone
    "Compatibility shim for Pharo <11, where #waitForSendDone: returns a boolean
    indicating success, rather than signaling exceptions on failure.
    A true return (in P11) or nothing/self (in P12) both indicate success, so check for specifically false."

    (socket waitForSendDoneFor: self timeout) == false ifTrue: [
        ConnectionTimedOut signal: 'Data send timed out.' ]

If that seems like the best solution to you @svenvc I can update this PR.

[daniels220]

Oh, and since this seems to have turned into a confusing mess, let me add that some of the confusion is due to my neglecting to update the comment in waitForSendDoneFor: as part of pharo#15984, and then when I noticed and updated it as part of pharo#16073, that hit an unrelated snag, got reverted and then languished for a long time before I had a chance to get back to it, and now we're working through some test issues with the replacement pharo#16172. So hopefully everything will be squared away soon.

[daniels220]

Alright, @jbrichau with your vote in favor I figured I might as well make the works-in-either-environment change to socketWaitForSendDone.

@svenvc is everything clear wrt to what the old/new Pharo core implementations are, and do you agree that this is the correct way to handle compatibility? I must say I remain surprised Zinc hasn't needed to accommodate breaking changes to Pharo before, such that you'd have compat packages/branches/etc. But maybe the API surface area is small enough it just hasn't happened yet? If you want to do something else, let me know and I can work with it.

[svenvc]

Hi @daniels220

Thanks for all the effort. I agree with the last approach. Let's try and see what happens.

Sven

[daniels220]

The Pharo 12 release includes a backwards-compatible return value from waitForSendDone[For:...], but it seems like it would still be good to clean up Zodiac—that method will no longer ever return false, so the branch in socketWaitForSendDone is dead code in P12+.